MBS-12715, MBS-13526: ArtistCreditEditor refactor

[mwiencek]

This is based on top of #3111 since I use the new library for updating React state here.

Problem

The current ArtistCreditEditor has a number of issues:

• It has no Flow types.
• It's using old autocomplete/bubble components which also have no Flow types, and have been replaced by other components (Autocomplete2, ButtonPopover).
• It's using ugly hacks to keep the bubble in sync with the editor component, instead of createPortal.
• It's written as a class-based component, which React no longer recommends.
• It maintains its own internal React state which must be synchronized with other view models from the inside, instead of being stateless and fully controlled by parent components.
• It logs of bunch of warnings due to incorrect usage of flushSync.

In case the motivation is not clear: the above issues currently make it difficult to use the ArtistCreditEditor in future edit forms, like the React conversion of the release editor, and the "alternative tracklists" editor.

Solution

This basically refactors the entire component and its sub-components, and updates a lot of glue code that keeps it in sync with Knockout observables in the release, recording, and release group edit forms.

Testing

Just manual testing so far in the release, recording, and release group edit forms.

• Copying/pasting artist credits.
• Using "guess feat. artists" in the recording edit form.
• Moving between tracks in the release editor, including using the enter key.

[reosarevok]

Lots to go yet, few comments for now.

[mwiencek]

Finally got tests working here. Many of the failures revealed actual bugs, all of which I've fixed, but other failures were simply due to the HTML of the autocompletes changing.

[reosarevok]

Tested this locally, seems to work fine - except for the automatic feat. standardization, but that's good since that hasn't been our guideline for years now so that code should be removed anyway. Do check if other join phrase standardizations are still wanted though that also fail?

[reosarevok]

Some comments, checking this up to the start of the release-editor folder.

[reosarevok]

How does nonEmpty work on knockout observables? Would it always see it as non-empty even if the T in Observable<T> is ''?

[mwiencek]

It would always see it as non-empty, yes. But the nonEmpty call was mainly added to work around a sketchy-null error from Flow. If the unwrapped gid is empty, entityHref will return a nonsense result either way (because id will just remain an empty string).

[mwiencek]

I'm guessing that's why I originally added #3113 (comment) though.

[reosarevok]

There's a conflict here, but in general maybe we should put this on test?

[mwiencek]

There's a conflict here, but in general maybe we should put this on test?

Sure, I've rebased and put it on the test server now. :)

[reosarevok]

Few comments from testing on test.mb (mostly works just fine AFAICT, these are all comments or very small issues):

1. Is it intentional that the popover takes less space than before, and that "Done" is no longer right-aligned? (I assume the change from grey to white is intentional since it shows the buttons a lot more clearly)
2. Is it intentional that the entered credit is kept when blanking the artist field with the keyboard, but only if it differs from the default fro the artist being deleted? (seems useful but it differs from prod where it seemingly always gets removed when blanking the field, so making sure it's a feature and not a bug)
3. Do we store at all the [removed] entries in state? Would it be possible to un-remove if I click by mistake? (before it was annoying to misclick but you sighed and moved on... but now it reminds you it knows there was something, so it kinda feels you should be able to get it back!)
4. If I have at least two empty artist fields, I can end up with multiple Recent items dropdowns at once. It seems to mostly be reproduceable by clicking one empty field, clicking somewhere else, then clicking the other empty field, see:
   
5. Probably the weirdest nitpick ever, but in case it might show some other underlying issue: if I blank a join phrase between artists 1 and 2, then I add artist 3, the blanked phrase remains blanked. If I paste an AC with a blank join phrase between artists 1 and 2, then I add artist 3, the blanked phrase turns into , .
6. More nitpicking: Does the positioning of the search icon look weird to you in the release tracklist? It does to me, it kinda feels like it should be either also green so it looks like the artist field, or exactly in the center of the white square?
   
7. Semi-relatedly, should we grey out that search icon or have a hover text or something so that it is clearer that it will work for track 1 here but not track 2? (this is not a new issue but for some reason - probably because the icon is bigger now - it feels more annoying than before). It still shows "Search" on hover, maybe it could at least show "Cannot search for multiple artists" or something?
   

[yvanzo]

Also when removing a name, the line stays with [removed] and the red cross button, whereas in production the line just disappears.

[yvanzo]

ArtistCreditEditor refactor (including MBS-12715)

It still is the main change here so the usual MBS-12715: Refactor artist credit editor would do.

[reosarevok]

The [removed] thing is expected, it's apparently an accessibility thing (that's why the relationship editor already does it).

[yvanzo]

The [removed] thing is expected, it's apparently an accessibility thing (that's why the relationship editor already does it).

Ok, I guess that it is a first step towards your above point 3 about un-removing (as the relationship editor already does it).

[mwiencek]

Is it intentional that the popover takes less space than before

Do you mean more space? I had increased the max-width, and it's clearly bigger for me.

and that "Done" is no longer right-aligned?

Fixed

I assume the change from grey to white is intentional since it shows the buttons a lot more clearly

I changed it back to gray, since nobody complained about it before

Is it intentional that the entered credit is kept when blanking the artist field with the keyboard, but only if it differs from the default fro the artist being deleted? (seems useful but it differs from prod where it seemingly always gets removed when blanking the field, so making sure it's a feature and not a bug)

I'm not sure if this was intentional, but if you prefer the behavior we could keep it.

Do we store at all the [removed] entries in state? Would it be possible to un-remove if I click by mistake? (before it was annoying to misclick but you sighed and moved on... but now it reminds you it knows there was something, so it kinda feels you should be able to get it back!)

You can undo them now

If I have at least two empty artist fields, I can end up with multiple Recent items dropdowns at once. It seems to mostly be reproduceable by clicking one empty field, clicking somewhere else, then clicking the other empty field, see: [...]

Fixed

Probably the weirdest nitpick ever, but in case it might show some other underlying issue: if I blank a join phrase between artists 1 and 2, then I add artist 3, the blanked phrase remains blanked. If I paste an AC with a blank join phrase between artists 1 and 2, then I add artist 3, the blanked phrase turns into , .

Fixed

More nitpicking: Does the positioning of the search icon look weird to you in the release tracklist? It does to me, it kinda feels like it should be either also green so it looks like the artist field, or exactly in the center of the white square?

I tweaked the positioning and it turns green now. (However, this looks kinda weird for inputs that still have a border -- unlike the ones in the tracklist.)

Semi-relatedly, should we grey out that search icon or have a hover text or something so that it is clearer that it will work for track 1 here but not track 2? (this is not a new issue but for some reason - probably because the icon is bigger now - it feels more annoying than before). It still shows "Search" on hover, maybe it could at least show "Cannot search for multiple artists" or something?

This sounds like a good idea, but I'm not actually sure how to gray out the icon (and it's an Autocomplete2 issue, not an ArtistCreditEditor issue -- I'd prefer to work on other improvements later).

[yvanzo]

It looks good to me. I didn’t test the latest changes but you obviously did.

Just a couple of minor tidying-up points:

• “Removal undoing” at least seems to be worth a separate ticket.
• The PR title should start to with the addressed ticket(s) as usual, even though some more refactoring is done as well.

[mwiencek]

The PR title should start to with the addressed ticket(s) as usual, even though some more refactoring is done as well.
“Removal undoing” at least seems to be worth a separate ticket.

Apologies, since you'd suggested that previously in #3113 (comment) but I forgot to apply the changes.

I also opened MBS-13526 for the undo button.