[WIP] MBS-12758: Convert label edit form to React #3672
Draft
reosarevok wants to merge 36 commits intometabrainz:masterfrom
Draft
[WIP] MBS-12758: Convert label edit form to React #3672reosarevok wants to merge 36 commits intometabrainz:masterfrom
reosarevok wants to merge 36 commits intometabrainz:masterfrom
Conversation
reosarevok
force-pushed
the
MBS-12758
branch
2 times, most recently
from
cf5676e to
a51e72e
Compare
|
hey can i work on the issue?? |
reosarevok
force-pushed
the
MBS-12758
branch
4 times, most recently
from
e35d538 to
f1f1e74
Compare
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.
This modifies FormRowTextList to make it properly usable inside a larger form; rather than have its own state, it accepts dispatch and state props. This is needed to make it usable in the label form later. For uses where the FormRow still needs its own state (because it's just a standalone component used in a TT form) I added StandaloneFormRowTextListSimple, to which the old local state has moved.
This will be used in the label form. Since the label codes are actually numbers, stored as INTEGER and checked as numbers anyway, this supports StrOrNum, although the form validation will always probably only need string.
This will be used in the label form (and artist later on). This combines is_valid_ipi and format_ipi from Server::Validation since we don't want to reject IPIs 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 IPI on the form itself so I'm letting it happen at the Perl level as earlier.
This will be used in the label form (and artist later on). This combines is_valid_isni and format_isni from Server::Validation since we don't want to reject ISNIs 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 ISNI on the form itself so I'm letting it happen at the Perl level as earlier.
We will need an equivalent to this for many editors, so it makes sense for it to be a reusable component.
reosarevok
force-pushed
the
MBS-12758
branch
from
fbc7228 to
2c3b8f3
Compare
I wrote this and then I gave up on typing debounceComputed, but it cannot hurt to have I guess.
Since debounceComputed is a lot messier for types and we probably will drop it eventually, for now this just moves it to a separate file.
needs debouncing
When a seeded form is submitted with errors, it gets reloaded. Currently, that means the name param gets sent twice, both as a query param and as a body param, and it ends up being set as [$name, $name]. This ensures we just use one of the two. I haven't yet hit this issue with ordering_type_id but it works in the same way so doing the same thing there too.
reosarevok
force-pushed
the
MBS-12758
branch
from
2c3b8f3 to
ba725b8
Compare
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
4 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-12758
Description
This just converts the label form to React.
I'm moving all the validation to the JS level as with the recording editor. Submitting separate preparatory steps as their own commits to make them easier to review.
On top of #3670
Draft progress
Known to be missing:
label/edit_form.ttandstatic/scripts/label/edit.jsTesting
Tested adding and editing labels, including: