MBS-10742: Show right number of hidden events #1449

reosarevok · 2020-04-03T18:48:27Z

Fix MBS-10742

The number being printed in "show {n} more" was events.length - COLLAPSE_THRESHOLD, but in fact the code prints two events before the collapse and one after - 3, not the 4 for COLLAPSE_THRESHOLD.

This moves the numbers to (easier to understand) constants, and then ensures we show the right number of events.

mwiencek

Looks like root/static/scripts/common/components/WorkArtists.js needs fixing too, since the code was copied from here.

mwiencek · 2020-04-03T20:17:55Z

root/static/scripts/common/components/ReleaseEvents.js

@@ -123,7 +123,7 @@ const ReleaseEvents = ({
                  title={l('Show all release events')}
                >
                  {bracketedText(texp.l('show {n} more', {
-                    n: events.length - COLLAPSE_THRESHOLD,
+                    n: events.length - 3,


I'd still keep the constant though, just with the offset correction added:

Suggested change

n: events.length - 3,

n: events.length - COLLAPSE_THRESHOLD + 1,

I didn't because it seemed arbitrary, but maybe? If anything, it'd feel better to have events.length - (COLLAPSE_THRESHOLD - 2 + 1), since that's what we actually do. But I'll make the suggested change for now :)

I can see what you mean. That might be more clear, particularly if we had a named constant for the 2 like COLLAPSE_MIN or something. I'd mainly just like that if we changed COLLAPSE_THRESHOLD and nothing else, the code would still work. :)

reosarevok · 2020-04-04T08:31:36Z

WorkArtists is fine, since it doesn't cut before or after. It just shows COLLAPSE_THRESHOLD artists and then the text.

root/static/scripts/common/components/ReleaseEvents.js

mwiencek · 2020-04-14T18:16:01Z

root/static/scripts/common/components/ReleaseEvents.js

@@ -17,7 +17,9 @@ import isDateEmpty from '../utility/isDateEmpty';

 import EntityLink from './EntityLink';

-const COLLAPSE_THRESHOLD = 4;
+const TO_TRIGGER_COLLAPSE = 4;


But if you have TO_SHOW_BEFORE and TO_SHOW_AFTER, I'm not sure this makes sense to configure separately - I'd think you should just do const TO_TRIGGER_COLLAPSE = TO_SHOW_BEFORE + TO_SHOW_AFTER + 1 below (or + 2 rather, since you want to hide at least two, and since that's technically the "trigger," but you'll need to change > to >= below too in that case).

mwiencek · 2020-04-14T18:17:40Z

root/static/scripts/common/components/ReleaseEvents.js

+              {buildReleaseEventRow(
+                events[events.length - TO_SHOW_AFTER],
+                abbreviated,
+              )}


This needs to be changed to use a slice like TO_SHOW_BEFORE has, 'cause right now this does/means something different (an offset of a single item from the end)

I think it should be fine now!

root/static/scripts/common/components/WorkArtists.js

The number being printed in "show {n} more" was events.length - COLLAPSE_THRESHOLD, but in fact the code prints two events before the collapse and one after - 3, not the 4 for COLLAPSE_THRESHOLD. This moves the numbers to (easier to understand) constants, and then ensures we show the right number of events.

* master: Update POT files using the production database Update translations from Transifex MBS-10391: Convert Add Release Group edit to React (#1423) MBS-10677: Return entity types for WS entities in rels and ACs (#1415) Bump tablesorter to 2.31.3 (#1465) Clarify nominated days in cron scripts MBS-10546: Export search indexes dump to FTP MBS-10755: Validate Musik-Sammler.de URL rels (#1457) MBS-10740: Convert entity pair rel list to React MBS-10740: Convert rel attribute list page to React Use sortname instead of name in sortByEntityName when relevant Add missing sortByEntityName call to RecordingMerge Change sortByEntityName to use i18n/compare MBS-10741: Make "in use" pages consistent MBS-10740: Convert relationships in_use pages to React MBS-10742: Show right number of hidden events (#1449) Refactor: Remove unneeded $self from gpg_sign MBS-10740: Convert relationships index table to React MBS-10740: Convert relationships header to React MBS-10756: Remove link_order from rels sort (#1458) Standardise and improve merge page message MBS-10364: Convert work merge page to React MBS-10364: Convert series merge page to React MBS-10364: Convert RG merge page to React Don't use DescriptiveLink for place lists MBS-10364: Convert place merge page to React MBS-10364: Convert label merge page to React Ignore case for sorting entities being merged MBS-10364: Convert instrument merge page to React MBS-10364: Convert event merge page to React MBS-10364: Convert area merge page to React MBS-10719: Change Maybe to Optional on RemovePUID MBS-10751: Convert Remove PUID edit to React

* master: Update POT files using the production database Regenerate yarn.lock Update translations from Transifex Bump shelljs to 0.8.3 and shell_quote to 1.7.2 MBS-10772: Show top 5 genres and other tags on sidebar (#1468) Add a CSRF token test for the registration page MBS-10778: Add CSRF tokens to user/admin forms Add form_{posted,submitted}_and_valid to MusicBrainz::Server Remove unused subroutine Add React context for SanitizedCatalystContext Unhide and fix withCatalystContext errors Regenerate cpanfile.snapshot MBS-10717: Set SameSite=None on session cookie Update POT files using the production database Update translations from Transifex Add support for Traxsource URLs (#1476) Bump react to 16.13.1 Bump flow-bin to 0.123.0 Upgrade eslint and eslint plugins Add missing key prop to Fragment siblings (#1472) MBS-9086: Move most relationships away from area overview Return heading in fallback for Relationships table Update POT files using the production database Update translations from Transifex MBS-10391: Convert Add Release Group edit to React (#1423) MBS-10677: Return entity types for WS entities in rels and ACs (#1415) Disable flaky selenium test MBS-10359: Guess feat. adds bogus trailing join phrases Bump tablesorter to 2.31.3 (#1465) Clarify nominated days in cron scripts MBS-10546: Export search indexes dump to FTP MBS-10755: Validate Musik-Sammler.de URL rels (#1457) MBS-10740: Convert entity pair rel list to React MBS-10740: Convert rel attribute list page to React Use sortname instead of name in sortByEntityName when relevant Add missing sortByEntityName call to RecordingMerge Change sortByEntityName to use i18n/compare MBS-10741: Make "in use" pages consistent MBS-10740: Convert relationships in_use pages to React MBS-10742: Show right number of hidden events (#1449) Refactor: Remove unneeded $self from gpg_sign MBS-10740: Convert relationships index table to React MBS-10740: Convert relationships header to React MBS-10756: Remove link_order from rels sort (#1458) Standardise and improve merge page message MBS-10364: Convert work merge page to React MBS-10364: Convert series merge page to React MBS-10364: Convert RG merge page to React Don't use DescriptiveLink for place lists MBS-10364: Convert place merge page to React MBS-10364: Convert label merge page to React Ignore case for sorting entities being merged MBS-10364: Convert instrument merge page to React MBS-10364: Convert event merge page to React MBS-10364: Convert area merge page to React MBS-10719: Change Maybe to Optional on RemovePUID MBS-10751: Convert Remove PUID edit to React Eslint autofixes for flowtype/delimiter-dangle MBS-1921: Display edit link under annotations

reosarevok added the Bug Bugs that should be checked/fixed soonish label Apr 3, 2020

reosarevok requested review from mwiencek and yvanzo April 3, 2020 18:48

mwiencek requested changes Apr 3, 2020

View reviewed changes

reosarevok force-pushed the MBS-10742 branch from f18b165 to 57dcc95 Compare April 4, 2020 08:31

reosarevok force-pushed the MBS-10742 branch from 57dcc95 to 22b79cf Compare April 6, 2020 08:47

yvanzo reviewed Apr 8, 2020

View reviewed changes

root/static/scripts/common/components/ReleaseEvents.js Outdated Show resolved Hide resolved

reosarevok force-pushed the MBS-10742 branch 2 times, most recently from 9d577c2 to d77abdc Compare April 14, 2020 10:58

mwiencek reviewed Apr 14, 2020

View reviewed changes

reosarevok force-pushed the MBS-10742 branch from d77abdc to d76ceae Compare April 14, 2020 20:14

yvanzo requested changes Apr 14, 2020

View reviewed changes

root/static/scripts/common/components/WorkArtists.js Outdated Show resolved Hide resolved

reosarevok force-pushed the MBS-10742 branch from d76ceae to 1296ac6 Compare April 14, 2020 20:48

yvanzo approved these changes Apr 14, 2020

View reviewed changes

mwiencek approved these changes Apr 14, 2020

View reviewed changes

reosarevok merged commit 435d0e5 into metabrainz:master Apr 15, 2020

reosarevok deleted the MBS-10742 branch April 15, 2020 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBS-10742: Show right number of hidden events #1449

MBS-10742: Show right number of hidden events #1449

reosarevok commented Apr 3, 2020 •

edited

mwiencek left a comment

mwiencek Apr 3, 2020

reosarevok Apr 4, 2020

mwiencek Apr 5, 2020

reosarevok commented Apr 4, 2020

mwiencek Apr 14, 2020

mwiencek Apr 14, 2020

reosarevok Apr 14, 2020

	n: events.length - 3,
	n: events.length - COLLAPSE_THRESHOLD + 1,

MBS-10742: Show right number of hidden events #1449

MBS-10742: Show right number of hidden events #1449

Conversation

reosarevok commented Apr 3, 2020 • edited

Fix MBS-10742

mwiencek left a comment

Choose a reason for hiding this comment

mwiencek Apr 3, 2020

Choose a reason for hiding this comment

reosarevok Apr 4, 2020

Choose a reason for hiding this comment

mwiencek Apr 5, 2020

Choose a reason for hiding this comment

reosarevok commented Apr 4, 2020

mwiencek Apr 14, 2020

Choose a reason for hiding this comment

mwiencek Apr 14, 2020

Choose a reason for hiding this comment

reosarevok Apr 14, 2020

Choose a reason for hiding this comment

reosarevok commented Apr 3, 2020 •

edited