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.

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

#133: Disable upload song button while processing request #250

Conversation

ObuMan
Copy link
Contributor

@ObuMan ObuMan commented Oct 7, 2024

Description

  • Added disabled={loadingUploadSong} to the song upload button.
  • To manually check if the button really becomes unavailable when uploading a song, I made a post request to the route http://127.0.0.1:8000/artists/?name=ArtistName&photo=http://photo.jpg&password=Password, thereby creating a new user with the role 'Artist'. Next step I logged in as the user I just created. And already from this account I uploaded the song and checked whether the button really became unavailable for pressing.
  • Created a unit test to test the behavior of the button

Commit type

feat: indicates the addition of a new feature or functionality to the project.

Issue

Modify the behaviour of upload song button so its disabled while waiting for the song to be uploaded.

Solution

Added disabled={loadingUploadSong} to the song upload button

Proposed Changes

  • Added disabled={loadingUploadSong} to the song upload button
  • A test to check if the button is really unavailable while the song is loading

Potential Impact

Screenshots

Additional Tasks

Assigned

@AntonioMrtz

@ObuMan ObuMan requested a review from AntonioMrtz as a code owner October 7, 2024 12:48
@AntonioMrtz
Copy link
Owner

Hi @ObuMan , thanks for your time . I Will check the PR tomorrow :)

@AntonioMrtz AntonioMrtz changed the title Feat/133 disable upload song button while processing request #133: Disable upload song button while processing request Oct 8, 2024
Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment

Choose a reason for hiding this comment

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

Hi @ObuMan , code looks good. I runned the pipelines and:

  • Run npm run format:write so the format pipeline doesn't fail.
  • Run npm run lint to check the error we're getting from the linter. You can see the ouput here

Thanks for your time and keeping the format and workflow of the contributions. Are you interested in taking more issues? If you find this project interesting/helpful, I'd appreciate it if you could star the repo!

@ObuMan
Copy link
Contributor Author

ObuMan commented Oct 11, 2024

Hi, I just noticed that some checks were not successful. I am trying to fix it.

@ObuMan
Copy link
Contributor Author

ObuMan commented Oct 11, 2024

At the expense of more issues. Yes, I would be very interested in that

@AntonioMrtz
Copy link
Owner

Hi @ObuMan , I'm still seeing errors in pipeline:

Lint

/home/runner/work/SpotifyElectron/SpotifyElectron/Electron/src/__tests__/components/Sidebar/AddSongPlayListAccordion.test.tsx
Error:   2:34  error  'waitFor' is defined but never used  @typescript-eslint/no-unused-vars

Format

Checking formatting...
[warn] src/__tests__/components/Sidebar/AddSongPlayListAccordion.test.tsx
[warn] Code style issues found in the above file. Run Prettier to fix.

@AntonioMrtz AntonioMrtz linked an issue Oct 12, 2024 that may be closed by this pull request
@AntonioMrtz
Copy link
Owner

@ObuMan Lint pipeline is successful but style is not. Run npm run format:write.

Checking formatting...
[warn] src/__tests__/components/Sidebar/AddSongPlayListAccordion.test.tsx
[warn] Code style issues found in the above file. Run Prettier to fix.

@AntonioMrtz
Copy link
Owner

Hi @ObuMan . I will check the final version tomorrow :). Thanks for your time

@AntonioMrtz
Copy link
Owner

HI @ObuMan , everything looks good :) . I will merge the PR as soon as possible. Maybe you want to check #255 , it's an small quality of life adjustment to the Playlist section. Let me know what you think 👍

@AntonioMrtz AntonioMrtz merged commit ee78d48 into AntonioMrtz:master Oct 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable upload song button while processing request
2 participants