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

MBS-13241: Prevent recording bubble from shifting. #3021

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

derat
Copy link
Contributor

@derat derat commented Aug 21, 2023

MBS-13241

Set "vertical-align: top" on the tracklist table rows at /release/add#recordings to prevent the "Edit" button from shifting vertically depending on the height of the track and recording titles, which was causing the bubble that contains the recordings list also shift vertically.

Also set "vertical-align: top" on the recording rows since it looks odd when the artist name and duration aren't aligned with the top of a multiline recording title and disambiguation.

Problem

The bubble that lists recordings often moves vertically when a recording is clicked, which can be disorienting. This happens because the bubble is aligned with the "Edit" button, which is vertically centered within its row, and the row's height can increase when the selected recording has a long title or disambiguation.

The vertical alignment of text within the tracklist and recordings list also looks a bit strange due to each cell being vertically centered.

Solution

Add CSS to ensure that each cell's contents are vertically aligned with the top of the row.

Before:
before

After:
after

Testing

I've been opening /release/add?release-group=0da580f2-6768-498f-af9d-2becaddf15e0 and adding a single "Fight Fire With Fire" track with length 4:45. There are plenty of recordings to select, some of which have long disambiguations. Without this change, the bubble moves vertically when clicking between recording without a disambiguation and one with a disambiguation. With the change, it (usually) doesn't move.

As described on the ticket, I've noticed that the bubble sometimes still shifts when I click a recording while scrolled all the way to the bottom of the page. I haven't been able to find any JS that's responsible for this and suspect that it may be builtin behavior in Chrome. I haven't had a chance to test whether the same thing happens in Firefox or not.

I won't be able to test this PR locally until next week, so I've just been using DevTools in Chrome 115 and Firefox ESR to apply these rules to the beta site and verify that they work as expected.

Set "vertical-align: top" on the tracklist table rows at
/release/add#recordings to prevent the "Edit" button from
shifting vertically depending on the height of the track and
recording titles, which was causing the bubble that contains
the recordings list also shift vertically.

Also set "vertical-align: top" on the recording rows since
it looks odd when the artist name and duration aren't
aligned with the top of a multiline recording title and
disambiguation.
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! The CSS changes look good to me.

I tried to test it locally but the sample database doesn’t have a lot of suggested recordings. It has to be tested with the full database. It is a limited change thus merging to beta now. 🚢

@yvanzo yvanzo merged commit 128764a into metabrainz:master Aug 21, 2023
1 of 2 checks passed
yvanzo added a commit that referenced this pull request Aug 21, 2023
* master:
  MBS-13241: Prevent recording bubble from shifting. (#3021)
@derat derat deleted the rec_align branch August 21, 2023 13:25
@derat
Copy link
Contributor Author

derat commented Aug 21, 2023

Thanks for the quick review and merge!

I think I might make a small followup change to tweak the cells' vertical padding a bit more. It looks like td.name and td.length both have padding-top: 1em, but the number on the left (td.position) and the edit button (td.buttons) don't, so they end up flush with the top of the cell now. Sorry for not noticing this earlier:

image

@yvanzo
Copy link
Contributor

yvanzo commented Aug 21, 2023

No problem. Would you prefer to have it deployed on test.mb.o instead?

@derat
Copy link
Contributor Author

derat commented Aug 21, 2023

No problem. Would you prefer to have it deployed on test.mb.o instead?

I'm happy with whatever you recommend! I've created #3022.

@yvanzo
Copy link
Contributor

yvanzo commented Aug 21, 2023

That was just in case it was likely to require more iterations, but it doesn’t seem to be needed, so I deployed #3022 directly to beta.

yvanzo added a commit that referenced this pull request Aug 28, 2023
* beta:
  Translated using Weblate (French)
  Translated using Weblate (Italian)
  Translated using Weblate (German)
  Translated using Weblate (German)
  Translated using Weblate (French)
  Update translation files
  Translated using Weblate (Croatian)
  Show the new tag icon for genre’s primary tag
  Add CSS classes for new collection/genre/tag icons
  MBS-12617, MBS-12618, MBS-12627 - New icons
  Translated using Weblate (French)
  Translated using Weblate (German)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Add top padding to track/recording position and buttons. (#3022)
  MBS-13241: Prevent recording bubble from shifting. (#3021)
  Translated using Weblate (French)
  MBS-13240: Update Mauritania flag (changed in 2017) (#3020)
  Translated using Weblate (Dutch)
  Translated using Weblate (Spanish)
  Translated using Weblate (German)
  Update translation files
  Translated using Weblate (Dutch)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Update translation files
  Translated using Weblate (German)
  Translated using Weblate (French)
  Add Hebrew to UI languages in beta
  Add Italian language to Docker development config
  Update POT files using the latest changes
  Update POT files using the latest changes
  MBS-13225: Add deps to useEffect call in ConfirmSeedButtons. (#3018)
  MBS-2604: Infer link direction between people and groups (#3011)
  MBS-13234: Handle international Ticketmaster URLs
  MBS-13235: Handle international Live Nation URLs
  Amend ce568d1: Update all "Username:" labels
  Amend 3b3b83b: Typo: Drop duplicated word
  Partly revert 9a432d2 committing CB PO files
  Translated using Weblate (Croatian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified))
  Update translation files
  Update POT files using the production database
  Translated using Weblate (German)
  Translated using Weblate (French)
  Update translation files
  Update POT files using the production database
  Translated using Weblate (Chinese (Simplified))
  Add main translation languages from Transifex
  Update latest translations from Transifex
  Revert "Update POT files using the production database"
  Update POT files using the production database
  MBS-13235: Add Live Nation links to sidebar
  MBS-13235: Handle livenation links
  MBS-13234: Add Ticketmaster links to sidebar
  MBS-13234: Handle ticketmaster links
  Support new ticketing reltypes
  MBS-13214: Clean Open Library URLs with no trailing slash (#3016)
  MBS-13225: Allow skipping Confirm Form Submission page (#3010)
  MBS-13238: Add validation for Open Library links
  MBS-13238: Also clean up Open Library publisher links
  MBS-13214: Also clean OpenLibrary URLs without slash after ID
  MBS-13165: Allow X ending for IdRef IDs (#3014)
  MBS-13237: Clean up Twitch mobile links (#3012)
  MBS-13207: Make release recording input not steal focus (#3008)
  MBS-12720: Set the remember_login cookie to HttpOnly (#2747)
  MBS-13208: Clean up Juno Download URLs further
  Remove unneded cases in Mainly Norfolk select
  MBS-13218: Allow linking Jaxsta works as lyrics
  MBS-13218: Show Jaxsta links in the sidebar
  MBS-13218: Add Jaxsta to other databases whitelist
  MBS-13209: Add validation for Juno Download URLs
  MBS-13208: Clean up Juno Download URLs
  MBS-10977: Handle Dribbble links
  MBS-13213: Handle Artstation links
  MBS-13212: Handle Behance links
  MBS-13211: Add pixiv links to sidebar
  MBS-13211: Handle pixiv links
  MBS-13210: Add DeviantArt links to sidebar
  MBS-13210: Handle DeviantArt links
  Support new art gallery artist-url reltype
  Remove unwanted console.log
  MBS-13185: Handle LibriVox URLs
  MBS-13227: Omit "quoted in" work-work rels in tracklists (#3009)
  Amend 717d2a0: Remind that Perl $! is $OS_ERROR
  Amend ab00262: Rename Perl $! to $OS_ERROR
  MBS-11947: Stop hiding focus indicators on links. (#3005)
  Script::JSONDump: Don't rmtree tmp_export_dir
  Allow creating extra temporary directories
  Allow specifying the path to temporary directory
  MBS-13222: Show MobyGames links in the sidebar
  MBS-13222: Add MobyGames to other databases whitelist
  MBS-13220: Show AniDB links in the sidebar
  MBS-13220: Add AniDB to other databases whitelist
  MBS-13215 - Update the Twitter/X logo (#2998)
  Amend 42cd50f: Initialize var to address warning (#2999)
  MBS-13216: Show VNDB links in the sidebar
  MBS-13216: Add VNDB to other databases whitelist
  MBS-13204: Show Goodreads links in the sidebar
  MBS-13204: Add Goodreads to other databases whitelist
  MBS-13202: Show LibraryThing links in the sidebar
  MBS-13202: Add LibraryThing to other databases whitelist
  Wrap long track names and ACs in MediumTracklist
  MBS-12948: Wrap long source entity names in rel dialog
  MBS-13051: Wrap long related entity names in release page
  MBS-13051: Wrap long track names and ACs in release page
  MBS-12948: Wrap long entity track names in rels editor
  MBS-12948: Wrap long entity names in rels editor relationships
  MBS-12948: Wrap long entity names in rel dialog preview
yvanzo added a commit that referenced this pull request Oct 17, 2023
* metabrainz/translations:
  Translated using Weblate (Portuguese)
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Russian)
  Translated using Weblate (French)
  Translated using Weblate (Bengali)
  Translated using Weblate (Portuguese)
  Translated using Weblate (German)
  Translated using Weblate (Italian)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (Spanish)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (French)
  Update translation files
  Update POT file from the current database
  MBS-13281: Don't check for mail confirmation for beginners (#3044)
  MBS-13072: Notify on subsecond recording edit lengths (#3032)
  MBS-13279: Don't consider the empty string an email address (#3041)
  MBS-13275: Don't send subscription emails to spammers (#3045)
  MBS-13155: Don't consider all unselected labels dupes (#3046)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (Spanish)
  Translated using Weblate (Italian)
  Update translation files
  Reject disallowed nomination with 403, not 404
  MBS-13280: Block nominating unconfirmed editors
  Clarify translation steps for beta release (#3040)
  Update POT files using the production database
  MBS-12786: Drop CD Stub adding/editing code (#2780)
  MBS-13192: Redirect to profile post report rather than detach (#3036)
  MBS-13252: Report: Show notes links on non-Broadcast releases (#3037)
  MBS-13268: Stop cleaning up CDBaby links (#3029)
  MBS-11920: Report for releases with low data quality (#2503)
  MBS-12423: Block VGMdb links from release groups (#3034)
  MBS-13278: Show label type descriptions on edit form (#3038)
  MBS-13148: Update Amazon Music favicon (#3030)
  MBS-13195: More useful sort for applications list (#3031)
  Translated using Weblate (Lithuanian)
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Italian)
  Add tests for release filtering
  MBS-12460: Allow filtering artist releases by status
  MBS-12269: Allow filtering artist releases by label
  Only get non-visible collection count, not contents
  MBS-12622: Show if URL has pending edits in links editor
  MBS-12622: Show if URL rel has pending edits in links editor
  MBS-12047: Support browsing artist RGs in /ws/2 as in website index (tests)
  MBS-12047: Support browsing artist RGs in /ws/2 as in website index
  MBS-13145: Also show collection tab to logged out viewers
  MBS-13244: Add standalone recording filter to artist index
  Merge find_video / find_standalone into default
  Update translation files
  Translated using Weblate (Hebrew)
  Translated using Weblate (Italian)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Update POT file from the current database
  admin/run: Ignore Shellcheck false positives
  admin/run: Run self as musicbrainz if possible
  admin/run: Exit on error if no musicbrainz user
  admin/run: Always check ~musicbrainz for log path
  Add script to create log directories in production
  Amend c52bd28: Cut down wording (from aerozol)
  Revert "admin/run: fix log path and automatically create it"
  MBS-3629: Add check/uncheck all option to subscription pages (#3025)
  MBS-13250: Don't crash on missing text content on setlist (#3023)
  MUSICBRAINZ_RUNNING_TESTS needed for Selenium JS
  Add test for user restrictions display
  Fix nonsensical "collections tab" test
  MBS-13224: Show edit/note restrictions from user profile
  Change URL to invite helping with translations
  Drop now unneeded Transifex config and script
  Update the docs for releasing with Weblate
  MBS-10629: Show Anghami links on the sidebar
  MBS-10629: Add cleanup and validation for Anghami URLs
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Dutch)
  Translated using Weblate (German)
  MBS-13260: Upgrade Node.js to v18
  Bump chromedriver to 116.0.5845.96
  Bump chrome-remote-interface to 0.33.0
  Launch chrome test instance with about:blank
  Fix some new BuiltinFunctions::ProhibitVoidMap violations
  Translated using Weblate (French)
  Translated using Weblate (Italian)
  Translated using Weblate (German)
  Translated using Weblate (German)
  Translated using Weblate (French)
  Update translation files
  Translated using Weblate (Croatian)
  Show the new tag icon for genre’s primary tag
  Add CSS classes for new collection/genre/tag icons
  MBS-12617, MBS-12618, MBS-12627 - New icons
  Translated using Weblate (French)
  Translated using Weblate (German)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Add top padding to track/recording position and buttons. (#3022)
  MBS-13241: Prevent recording bubble from shifting. (#3021)
  Translated using Weblate (French)
  MBS-13240: Update Mauritania flag (changed in 2017) (#3020)
  Translated using Weblate (Dutch)
  Translated using Weblate (Spanish)
  Translated using Weblate (German)
  Update translation files
  Translated using Weblate (Dutch)
  Translated using Weblate (French)
  Translated using Weblate (German)
  Update translation files
  Translated using Weblate (German)
  Translated using Weblate (French)
  Add Hebrew to UI languages in beta
  Add Italian language to Docker development config
  Update POT files using the latest changes
  Update POT files using the latest changes
  MBS-13225: Add deps to useEffect call in ConfirmSeedButtons. (#3018)
  MBS-2604: Infer link direction between people and groups (#3011)
  MBS-13234: Handle international Ticketmaster URLs
  MBS-13235: Handle international Live Nation URLs
  Amend ce568d1: Update all "Username:" labels
  Amend 3b3b83b: Typo: Drop duplicated word
  Partly revert 9a432d2 committing CB PO files
  Translated using Weblate (Croatian)
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Simplified))
  Update translation files
  Update POT files using the production database
  Translated using Weblate (German)
  Translated using Weblate (French)
  Update translation files
  Update POT files using the production database
  Translated using Weblate (Chinese (Simplified))
  Add main translation languages from Transifex
  Update latest translations from Transifex
  Revert "Update POT files using the production database"
  Update POT files using the production database
  MBS-13235: Add Live Nation links to sidebar
  MBS-13235: Handle livenation links
  MBS-13234: Add Ticketmaster links to sidebar
  MBS-13234: Handle ticketmaster links
  Support new ticketing reltypes
  MBS-13214: Clean Open Library URLs with no trailing slash (#3016)
  MBS-13225: Allow skipping Confirm Form Submission page (#3010)
  MBS-13238: Add validation for Open Library links
  MBS-13238: Also clean up Open Library publisher links
  MBS-13214: Also clean OpenLibrary URLs without slash after ID
  MBS-13165: Allow X ending for IdRef IDs (#3014)
  MBS-13237: Clean up Twitch mobile links (#3012)
  MBS-13207: Make release recording input not steal focus (#3008)
  MBS-12720: Set the remember_login cookie to HttpOnly (#2747)
  MBS-13208: Clean up Juno Download URLs further
  Remove unneded cases in Mainly Norfolk select
  MBS-13218: Allow linking Jaxsta works as lyrics
  MBS-13218: Show Jaxsta links in the sidebar
  MBS-13218: Add Jaxsta to other databases whitelist
  MBS-13209: Add validation for Juno Download URLs
  MBS-13208: Clean up Juno Download URLs
  MBS-10977: Handle Dribbble links
  MBS-13213: Handle Artstation links
  MBS-13212: Handle Behance links
  MBS-13211: Add pixiv links to sidebar
  MBS-13211: Handle pixiv links
  MBS-13210: Add DeviantArt links to sidebar
  MBS-13210: Handle DeviantArt links
  Support new art gallery artist-url reltype
  Remove unwanted console.log
  MBS-13185: Handle LibriVox URLs
  MBS-13227: Omit "quoted in" work-work rels in tracklists (#3009)
  Amend 717d2a0: Remind that Perl $! is $OS_ERROR
  Amend ab00262: Rename Perl $! to $OS_ERROR
  MBS-11947: Stop hiding focus indicators on links. (#3005)
  Script::JSONDump: Don't rmtree tmp_export_dir
  Allow creating extra temporary directories
  Allow specifying the path to temporary directory
  MBS-13222: Show MobyGames links in the sidebar
  MBS-13222: Add MobyGames to other databases whitelist
  MBS-13220: Show AniDB links in the sidebar
  MBS-13220: Add AniDB to other databases whitelist
  MBS-13215 - Update the Twitter/X logo (#2998)
  Amend 42cd50f: Initialize var to address warning (#2999)
  MBS-13216: Show VNDB links in the sidebar
  MBS-13216: Add VNDB to other databases whitelist
  MBS-13204: Show Goodreads links in the sidebar
  MBS-13204: Add Goodreads to other databases whitelist
  MBS-13202: Show LibraryThing links in the sidebar
  MBS-13202: Add LibraryThing to other databases whitelist
  MBS-13098: Process cover art params for seeding (#2977)
  Simplify anyCombinationOf
  MBS-13169: Use SVG over PNG for new icons (#2984)
  MBS-13079: Add statistics for release groups with no type set (#2936)
  MBS-12886: Restrict Bandcamp /(album|track) to "get the music" rels
  Widen the combination options for Soundcloud URLs
  Wrap long track names and ACs in MediumTracklist
  MBS-12948: Wrap long source entity names in rel dialog
  MBS-13051: Wrap long related entity names in release page
  MBS-13051: Wrap long track names and ACs in release page
  MBS-12948: Wrap long entity track names in rels editor
  MBS-12948: Wrap long entity names in rels editor relationships
  MBS-12948: Wrap long entity names in rel dialog preview
  admin/run: fix log path and automatically create it
  Refactor: Rename `$*_sql` flag variables
  Refactor: Rename `$mirror` to `$applying_change_sql`
  Refactor: Rename `$sql2` to `$feching_change_sql`
  Add comments to processing replication changes
  DRY update-date-period actions
  MBS-12964: Allow specifying dates when batch-creating works
  Remove duplicate test
  Test WS validation
  Add basic test for event tags
  Add basic test for event index
  Add basic test for area events list with containment
  Add basic test for area places list with containment
  Document that both %20 and plus sign are allowed for inc combining
  Add basic test SQL for events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants