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

Convert the Release index page to React + add track paging #2131

Merged
merged 9 commits into from
Jun 18, 2021

Conversation

mwiencek
Copy link
Member

@mwiencek mwiencek commented Jun 7, 2021

Fixes MBS-9921
Fixes MBS-8844
Fixes MBS-11703

In addition to the 10-medium limit we've already had, I've added a 100 track limit, which covers the case of absurdly large mediums. When a medium like that is expanded, the track requests are paged (via a new endpoint, /ws/js/tracks) and can be fetched 100 at a time. I provided a no-script fallback as before.

@mwiencek mwiencek force-pushed the react-release-index branch 5 times, most recently from 4195e9b to 6bdd675 Compare June 9, 2021 06:46
@mwiencek mwiencek changed the title [WIP] Convert the Release index page to React + add track paging Convert the Release index page to React + add track paging Jun 9, 2021
@mwiencek mwiencek force-pushed the react-release-index branch 8 times, most recently from de8dd10 to 4c79083 Compare June 11, 2021 01:59
@reosarevok
Copy link
Member

One thing I just considered - do we know whether there are unloaded tracks still from the release index? Assuming we do, which I hope so, we should give some warning that there might be more unloaded relationships when viewing rels in "view at bottom".

@reosarevok
Copy link
Member

This might be expected for now, but just in case:

Warning: Text content did not match. Server: "Pealkiri" Client: "Title"
    at th
    at tr
    at tbody
    at table
    at http://localhost:5000/static/build/release/index.js:1416:26
    at http://localhost:5000/static/build/release/index.js:2118:24

@reosarevok
Copy link
Member

Also: on http://localhost:5000/release/b5d024d2-ca9a-46a7-887c-301bce20294f I get "This medium has too many tracks to load at once; currently showing 100 out of 512 total. Näita rohkem..." but clicking the show more / näita rohkem brings a "Laadimine..." / Loading notice up, but then nothing happens.

@mwiencek mwiencek force-pushed the react-release-index branch from 4c79083 to f2e37e2 Compare June 11, 2021 17:19
@mwiencek
Copy link
Member Author

One thing I just considered - do we know whether there are unloaded tracks still from the release index? Assuming we do, which I hope so, we should give some warning that there might be more unloaded relationships when viewing rels in "view at bottom".

Yeah, I added a message for that. Let me know if it looks okay.

This might be expected for now, but just in case:

This should be fixed once we regenerate POT files.

Also: on http://localhost:5000/release/b5d024d2-ca9a-46a7-887c-301bce20294f I get "This medium has too many tracks to load at once; currently showing 100 out of 512 total. Näita rohkem..." but clicking the show more / näita rohkem brings a "Laadimine..." / Loading notice up, but then nothing happens.

Fixed. I broke that somehow in a recent push.

@reosarevok
Copy link
Member

Fairly minor, but:

Screenshot from 2021-06-15 14-43-14

This seems to be a lot taller than it needs to be (thanks for adding it, in any case!).

The extra loading seems to not be loading work artists - the first one does though. See:

Screenshot from 2021-06-15 14-44-48

Also, for:

Screenshot from 2021-06-15 14-45-07

Would it make sense to have two options, load 100 more / load until all are loaded? With the caveat that the latter takes longer. I can't imagine clicking load 10 times for a 1000 track release, but I'd happily click load once, then go navigate elsewhere while it loads.

Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the work rels bit is working, this seems ready for beta at least. Other comments are fairly minor possible improvements.

@mwiencek mwiencek force-pushed the react-release-index branch 4 times, most recently from 8138228 to e9b7f98 Compare June 16, 2021 19:41
Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through + tested, seems ok now. Since it's a big change, let's put it in beta as soon as possible so it is there as long as possible :)

mwiencek added 4 commits June 16, 2021 15:13
Pass the required information (linkType, attributes) directly to the
linkPhrase functions rather than the whole relationship itself. This
makes typing easier in the new relationship editor, since the linkType
may be null on relationship objects while editing, but we can't refine
that fact (as in refining the Flow type) outside of linkPhrase.js.
This is broken because it doesn't take into account that the
entity0/entity1 parameters can change. Adding those to the cache key
isn't simple, because they're opaque React elements.

I don't think this cache will be all that useful in the new relationship
editor, in any case. `React.memo` will generally be used to prevent
components from re-rendering where relationships haven't changed.
This causes an exception where the json5 contains non-ASCII characters:

  TypeError: Cannot read property 'length' of undefined at ucs2decode
These fields take up a lot of space in our entity JSON. I believe we can
significantly reduce our markup size (from hydrated element props) in
some cases by removing them.

I didn't check that the JavaScript version is equivalent (it's most
likely not), but I don't think it has to be perfect for what we're using
it for.
mwiencek added 5 commits June 16, 2021 15:13
In order to determine whether a comment was required, we were comparing
the entered name against an unaccented version of each "duplicate"
entity name, but we weren't first unaccenting the entered entity name.
These props cause unnecessary bloat in cases where no rating exists, or
no rating is loaded.
Fixes a duplicate React key bug.
If a release has more than 10 mediums, we currently load it completely
collapsed.  This modifies that behavior to always load up to 100 tracks
or 10 mediums, whichever is hit first.

One side effect of this is that we may now preload some discs even if
the release has more than 10 mediums (as long as the sum of the track
counts on those discs is under 100).

The more important effect, though, is that we don't preload
single-medium releases that have more than 100 tracks. This is half of
MBS-11703; the other half will be to page the tracks.
Fixes MBS-9921
Fixes MBS-8844
Fixes MBS-11703

In addition to the 10-medium limit we've already had, I've added a 100
track limit, which covers the case of absurdly large mediums. When a
medium like that is expanded, the track requests are paged (via a new
endpoint, /ws/js/tracks) and can be fetched 100 at a time. I provided a
no-script fallback as before.
@mwiencek mwiencek force-pushed the react-release-index branch from e9b7f98 to 34fbf7d Compare June 16, 2021 20:15
@mwiencek mwiencek merged commit ff0f08b into metabrainz:master Jun 18, 2021
@mwiencek mwiencek deleted the react-release-index branch June 18, 2021 02:11
@jesus2099
Copy link
Contributor

jesus2099 commented Jul 9, 2021

Very nice track paging system!

Hem, it may sound selfish userscript requirement but:

  • The Expand all mediums button is like button#expand-all-mediums
  • Each Expand medium arrow link is like a.expand-medium
  • But this new Load all tracks link has no obvious CSS selector

I still can use:

table.medium > tbody > tr:not(.subh):not(.even):not(.odd) > td[colspan] > a[href='#']

But it feels less safe on the long term.

Update

I created a ticket for that: https://tickets.metabrainz.org/browse/MBS-11781

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

Successfully merging this pull request may close these issues.

3 participants