#234 Disable upload button song and playlist if required fields not filled

[SaurabhGurde]

Description

This will disable song and playlist upload button if required field are not filled

Commit type

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

Issue

#234

Solution

Added logic in disable button user will able to click button only when required field are filled.

Screenshots

Playlist getting added

@AntonioMrtz

[SaurabhGurde]

@AntonioMrtz We dont need this validation bacause "formDataSong" and "formDataPlaylist" will always give true value as boolean value of any object is true in javascript.

[AntonioMrtz]

HI @SaurabhGurde thanks for following the guidelines. I would check the formData thing you're telling me. Can we have tests about the new functionality?

[AntonioMrtz]

Hi @SaurabhGurde , formDataSong and formDataPlaylist checks are indeed not necessary, can you modify the code?

I notice this little bug in the interface, where close to song file picker there's a * symbol that doesn't do anything right?

I also noticed that fields are missaligned because of the * symbols. Fields rows should be aligned

[SaurabhGurde]

Can i update tomorrow. its 11:30 pm here 😊

[AntonioMrtz]

Can i update tomorrow. its 11:30 pm here 😊

Yeah no problem @SaurabhGurde , take all the time you need

[SaurabhGurde]

@AntonioMrtz pushed new code. fixed alignment using * but invisible property. its hard to calculate exact space taken by * and to add margin-left according to that.

[AntonioMrtz]

Hi @SaurabhGurde , code looks good. I would add justify content: space between in the file picker/ genre row. That way you can get the row to take all the available space like this:

Also as I mentioned in the other PR, tests are failing. We should fix the failing tests and add new ones for the implemented functionality

[SaurabhGurde]

@AntonioMrtz plz check now

[AntonioMrtz]

Hi @SaurabhGurde code looks good. The final part is to add a unit tests in AddSongPlayListAccordion.test.tsx that checks the following for playlist and song upload:

• Upload button is disabled if required fields are not filled
• Upload button is enabled if required fields are filled

Let me know if you need anything about these unit tests implementation

[AntonioMrtz]

Hi @SaurabhGurde , I saw the last changes you made.

That test case was made by another contributor to prove that the button was enabled/disabled when uploading a song. Each test should try to assert only one functionality.

In our case, it will be better to create a separate unit tests only to prove that the button is disabled if the required fields are not filled. Apply this to both add song and add playlist forms. As a result we should have 4 new tests:

• Song, required fields not filled -> button disabled
• Playlist, required fields not filled -> button disabled
• Song, required fields filled -> button enabled
• Playlist, required fields filled -> button enabled

[SaurabhGurde]

Hi @SaurabhGurde , I saw the last changes you made.

That test case was made by another contributor to prove that the button was enabled/disabled when uploading a song. Each test should try to assert only one functionality.

In our case, it will be better to create a separate unit tests only to prove that the button is disabled if the required fields are not filled. Apply this to both add song and add playlist forms. As a result we should have 4 new tests:

• Song, required fields not filled -> button disabled
• Playlist, required fields not filled -> button disabled
• Song, required fields filled -> button enabled
• Playlist, required fields filled -> button enabled

i have modified one test case for upload song. button will be disabled by default and will only enable if required field are filled. and should i add test case for add playlist as well. or may be you are trying to say that i should remove test of disabled button during upload from song upload test and create another test for it "OR" should i completely remove test for disabled button during upload song?

[AntonioMrtz]

Hi @SaurabhGurde , I saw the last changes you made.
That test case was made by another contributor to prove that the button was enabled/disabled when uploading a song. Each test should try to assert only one functionality.
In our case, it will be better to create a separate unit tests only to prove that the button is disabled if the required fields are not filled. Apply this to both add song and add playlist forms. As a result we should have 4 new tests:

• Song, required fields not filled -> button disabled
• Playlist, required fields not filled -> button disabled
• Song, required fields filled -> button enabled
• Playlist, required fields filled -> button enabled

i have modified one test case for upload song. button will be disabled by default and will only enable if required field are filled. and should i add test case for add playlist as well. or may be you are trying to say that i should remove test of disabled button during upload from song upload test and create another test for it "OR" should i completely remove test for disabled button during upload song?

Hi, you should add the 4 new tests listed. We don't wanna modify the already existing disable button tests. So:

• Revert the modified tests from other contributor
• Add the 4 new tests listed

[SaurabhGurde]

@AntonioMrtz i have just pushed new test case for enabling upload button when required field are filled. plz check whether code format is correct or not that you want.

[AntonioMrtz]

@SaurabhGurde It looks good, just a few things:

• Change test name to be something like: Upload song form button disabled and enabled depending on filled fields
• Add the same test for playlists

:)

[SaurabhGurde]

@AntonioMrtz plz check added test for playlist button as well.

[AntonioMrtz]

Hi @SaurabhGurde everything looks god. I will do a last check launching the app and merge if everthing works as expected :)

[AntonioMrtz]

Hi @SaurabhGurde only some minor things:

When button is disabled boostrap adds some extra classes, the disabled button looks like this:

This style is more consitent with the app theme:

I only removed this styles(border and color):

[SaurabhGurde]

@AntonioMrtz

added globally. whenever we will add button anywhere in project and if its disabled then it will have consistant styling across project. it will override styling of bootstrap

[AntonioMrtz]

@AntonioMrtz added globally. whenever we will add button anywhere in project and if its disabled then it will have consistant styling across project. it will override styling of bootstrap

I dont think it's needed as global style yet. I'm planning on doing a refactor on frontend styles to make them more re-usable across the code. Can we just put the disabled button styles only for this component? Thanks, I will merge the PR when those changes are made

[SaurabhGurde]

@AntonioMrtz
before required field filled

After required field filled