MBS-12757 / MBS-14190: Convert instrument edit form to React #3670
Draft
reosarevok wants to merge 24 commits intometabrainz:masterfrom
Draft
MBS-12757 / MBS-14190: Convert instrument edit form to React #3670reosarevok wants to merge 24 commits intometabrainz:masterfrom
reosarevok wants to merge 24 commits intometabrainz:masterfrom
Conversation
This seems entirely unused.
This class being added to the non-hydrated components is actually confusing the hydrate utility and seems to have no actual use - we never call this selector anywhere AFAICT.
This was very useful shortly after NGS migration, but it hasn't been really needed for years since most of the old-style MB classical data has been fixed. New classical releases (from data outside MusicBrainz) do not use the old MB style this is built to fix at all, so there's no need to keep it and we can simplify the guess feat code.
We will need an updated guessFeat for the recording form on a subsequent commit. Since this makes a fair amount of changes and improves the tests, I'm splitting it into its own commit for ease of review. I'm not touching the existing knockout guessFeat, which will still be needed in the release editor and the RG form until those two get converted to React as well. Once that is complete, we can remove the last two functions of the guessFeat file and simplify the tests a bit.
This uses the new guessFeat introduced in the previous commit. This is an initial conversion; further commits introduce extra features like a lot more validation at the JS level before the user submits the form.
This removes the need for initializeValidation() and replaces it with a more proper React way of disabling submit using state. I made updateNameFieldErrors reusable so we can update the form when creating the state, otherwise the submit button won't get disabled until the user writes something in the name field.
This avoids blanking the artist credit when reloading the page on the recording creation page (now it will keep any AC the user had entered).
This can be triggered via `/recording/create?edit-recording.artist_credit.names.0.artist.id=foo`.
I'll next use it as part of a new isInvalidLength JS check.
Our unformatTrackLength JS implementation was a lot different from the Perl one. This worked fine when it was only used for the release editor, but now we want to also use it to test for validity at the JS level, so it should probably consider as valid the same things the Perl backend will. This commit mostly changes the unformatTrackLength JS version to work more like the Perl one. There's two things that the Perl version did not allow that probably make sense, so I've added them there: :SS times without minutes, and just parsing a number of seconds. I'm removing the test for 3723494 seconds from fields.js because that's actually a larger number in ms than we accept in the DB (and was being rejected by the new method). Similarly, :10 should now be an accepted track time, so changing that test to use a time that is actually still disallowed.
This modifies FormRowTextList to make it properly usable inside a larger form; rather than have its own state, it accepts dispatch and state props. Additionally, I'm moving currentTextValues outside the state since it is a constant, and just passing it as a prop. This was in the state for ease of use in the reducer, but it can just be passed by the dispatch as needed. For uses where the FormRow still needs its own state (because it's just a standalone component used in a TT form) I added StandaloneFormRowTextlist, to which the old local state has moved. I added isValidIsrc for JS-level ISRC validation; this combines is_valid_isrc and format_isrc from Server::Validation since we don't want to reject ISRCs that are valid once formatted by the system. I expect it'd be more confusing for the user to automatically change the format of the ISRC on the form itself so I'm letting it happen at the Perl level as earlier.
For creating standalone recordings, it makes sense to display an error saying a note is required from the get go. This additionally set errors for seeded bad edit notes.
The previous code invokes `createInitialState` on every single render. I noticed when changing the recording form that we still had these three around.
We don't intend to change nameStateCtx after assigning it to nameState in any of these forms, so it is safer to use final() to make sure it's not writable anymore.
useEffect causes an annoying flickering effect on changing the bubble position that seems to be avoided by useLayoutEffect. Thanks @mwiencek for the suggestion.
This is useful only for seeded lengths, but it makes sense to show an error for those when invalid.
I'm not showing any info bubbles in this editor since it is admin only, so we should not have a need for docs. I'm still keeping validation for empty name and invalid note since it's trivial and it might avoid us doing a dumb. I moved supportedHtmlTags to expand2react as per the comment in edit_form.tt.
reosarevok
changed the title
It is a lot nicer to be notified that your HTML is broken when you can still edit it, rather than seeing the broken HTML after submitting the instrument form. Since the error is caught and stashed as part of expand2, this ensures we also save the error message to the expand2 state so it can be retrieved and displayed to the user during validation.
reosarevok
force-pushed
the
MBS-12757
branch
from
ecf17ac to
c15efb1
Compare
5 tasks
|
hello |
Member
Author
|
This is basically done, please go to https://tickets.metabrainz.org/ and find an issue that hasn't been worked on yet. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement MBS-12757, MBS-14190
Description
I'm not showing any info bubbles in this editor since it is admin only, so we should not have a need for docs. I'm still keeping validation for empty name and invalid note since it's trivial and it might avoid us doing a dumb.
I moved
supportedHtmlTagstoexpand2reactas per the comment inedit_form.tt.On top of #3378
The second commit warns the user and blocks submission if the description field contains invalid HTML.
Testing
Tested adding and editing instruments, including:
Did not test seeding (not sure how often this would be seeded anyway) but it's using the same code that works for recording so it should work.