Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#228: Rename property and increase stream history length issue-number #281

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

#228: Rename property and increase stream history length issue-number #281

wants to merge 9 commits into from

Conversation

telepcak
Copy link

@telepcak telepcak commented Nov 3, 2024 •
edited by AntonioMrtz
Loading

Description

Backend variable name refractored, fixed corresponding test, proposed change to show only 5 songs on frontend

Commit type

Refactor

Issue

#228

Proposed Changes

Variable name is simply refractored and frontend change to show only 5 songs in profile history is achieved through boolean variable and button to show all songs or only 5

Potential Impact

Screenshots

Additional Tasks

Assigned

@AntonioMrtz

@telepcak telepcak requested a review from AntonioMrtz as a code owner November 3, 2024 19:49
@telepcak telepcak changed the title refractor: rename variable change, frontend fix to show 5 songs Issue: issue-title: Rename property and increase stream history length issue-number: #228 Nov 3, 2024
@AntonioMrtz
Copy link
Owner

Hi @telepcak , thanks for your time and contributions. I see you did both frontend and backend parts of the issue, that's nice. Here are some things I see from the pipeline and format:

  • Generate OpenAPI schema with new backend endpoint info. There's a docs section that explains how to do it.
  • Eslint pipeline fails. Check output. Should be a minor change, use keys that are unique and don't use index if possible.
  • PR name should look like this: #7777: Add Home Page as stated in docs

Let me know if you need anything :)

@AntonioMrtz AntonioMrtz linked an issue Nov 6, 2024 that may be closed by this pull request
Rename property and increase stream history length #228
Open
AntonioMrtz
AntonioMrtz requested changes Nov 6, 2024
View reviewed changes
Backend/app/spotify_electron/user/user_controller.py Outdated Show resolved Hide resolved
Backend/app/spotify_electron/user/user_controller.py Outdated Show resolved Hide resolved
Backend/tests/test__base_users.py Show resolved Hide resolved
Backend/tests/test__base_users.py Outdated Show resolved Hide resolved
Backend/tests/test_API/api_base_users.py Show resolved Hide resolved
Electron/src/pages/UserProfile/UserProfile.tsx Outdated Show resolved Hide resolved
Electron/src/pages/UserProfile/UserProfile.tsx Outdated Show resolved Hide resolved
Electron/src/pages/UserProfile/UserProfile.tsx Outdated Show resolved Hide resolved
Electron/src/pages/UserProfile/UserProfile.tsx Outdated Show resolved Hide resolved
@telepcak telepcak changed the title Issue: issue-title: Rename property and increase stream history length issue-number: #228 #228: Rename property and increase stream history length issue-number Nov 12, 2024
Tomáš Telepčák added 2 commits November 13, 2024 00:02
@telepcak
Copy link
Author

Hi, @AntonioMrtz , I hopefully fixed code according to proposed changes. Just leave a comment if sth still needs a change !

AntonioMrtz
AntonioMrtz requested changes Nov 13, 2024
View reviewed changes
Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your time. Code looks mostly good, but there's a few things we have to fix. We wanna get rid of playback word across the project so searching for this key word and refactor the naming convention should be done :) . Remember to star the project if you like it 😃

Electron/src/components/ShowAllItems/Items/ItemsAllSongsFromStreamHistory.tsx Outdated Show resolved Hide resolved
Backend/app/spotify_electron/user/base_user_service.py Outdated Show resolved Hide resolved
AntonioMrtz
AntonioMrtz requested changes Nov 13, 2024
View reviewed changes
Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks mostly good. Just a few minor fixes. When they're checked I would run the pipelines and do a last test with all the app launched. Can you check npm run lint and npm run test outputs? There're some adjustments need to be done there

@telepcak
Copy link
Author

npm run lint and npm run test both of them are missing SpotifyElectron/package.json file and it ends with error.

@AntonioMrtz
Copy link
Owner

AntonioMrtz commented Dec 1, 2024 •
edited
Loading

npm run lint and npm run test both of them are missing SpotifyElectron/package.json file and it ends with error.

Hi, you have to run npm commands from Electron folder, the package.json is located there :)

You can see outputs here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntonioMrtz AntonioMrtz AntonioMrtz requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@telepcak telepcak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename property and increase stream history length
2 participants
@telepcak @AntonioMrtz