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

fix BB-782 : Unable to edit Author Credit after having set it #1071

Closed
wants to merge 3 commits into from

Conversation

Tarunmeena0901
Copy link
Contributor

@Tarunmeena0901 Tarunmeena0901 commented Feb 26, 2024

Problem

BB-782

Solution

Based on the discussion in this issue link to issue, I'm still unable to unveil the mystery surrounding the isEditable checks. However, removing it seems to improve and make things less buggy, so perhaps we're better off without it.

Areas of Impact

author-credit-section.tsx

@Tarunmeena0901 Tarunmeena0901 changed the title fix(BB-782) - Unable to edit Author Credit after having set it fix BB-782 - Unable to edit Author Credit after having set it Feb 26, 2024
@Tarunmeena0901 Tarunmeena0901 changed the title fix BB-782 - Unable to edit Author Credit after having set it fix BB-782 : Unable to edit Author Credit after having set it Feb 26, 2024
@Tarunmeena0901
Copy link
Contributor Author

also looked at musicbrainz entity-editor and there is no problem like this... we can edit the artist credits as per our will

giving me another reason to believe that this was a bug

@MonkeyDo
Copy link
Contributor

The idea of isEditable here is to ensure users will not modify the text input directly after modifying in the author credit editor.

For the simplest case (single author, name identical), it makes it simple as typing the name to search then selecting one.
If you selected the wrong one you can simple type in the box again (isEditable is true).

However if you use the author credit editor for a more complex case (for example two authors), the resulting string representation should not be edited directly, so we disable the text input (isEditable=false) and force the user to use the AC editor instead

Does that make sense?

The same thing happens in MusicBrainz. For example, select an artist, then click "edit" to modify it (change the "Artist as credited" or add another artist) and you will see the text input becomes disabled straight away.

Closing this PR as I believe everything is working as expected, but if I misunderstood the issue do let me know and you can reopen the PR.

@MonkeyDo MonkeyDo closed this May 25, 2024
@Tarunmeena0901
Copy link
Contributor Author

Got it ✅✅ Now, as you explained, I think the main problem here is that after setting more than one author credit, the entire field and edit button get disabled. I think only the input field should be disabled, not the edit button, so users can still edit credits even after multiple credits are set.

image

@Tarunmeena0901
Copy link
Contributor Author

but again if that's the case then this PR wont solve this and I have to make some changes

@MonkeyDo
Copy link
Contributor

Yes you're absolutely right! I didn't see it was disabled once the AC is edited.
Indeed that's a bug and needs fixing, but probably best to start afresh in another PR.
Thanks for looking into it again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants