[WIP] MBS-12763: Convert RG edit form to React#3695
Draft
reosarevok wants to merge 26 commits intometabrainz:masterfrom
Draft
[WIP] MBS-12763: Convert RG edit form to React#3695reosarevok wants to merge 26 commits intometabrainz:masterfrom
reosarevok wants to merge 26 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.
This creates a reusable FormRowArtistCredit since we will want to do the same for RGs and probably releases.
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.
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.
secondary type select needs fiddling to allow multiselect [skip ci]
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
2 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-12763
Description
This just converts the release group form to React.
I'm moving all the validation to the JS level as with the recording editor.
On top of #3670
Draft progress
Known to be missing:
release_group/edit_form.ttandstatic/scripts/release_group/edit.jsTesting
None yet