MBS-13263: Align release titles despite indicators for cover art and data quality #3256
Conversation
|
Thanks for opening your first pull request for MusicBrainz Server, and welcome to our community! 🎉 |
|
Can one of the admins verify this patch? |
|
@brainzbot, add to whitelist |
|
This does seem to fix the ticket indeed (tested locally). That said, it seems strange to only change this for releases, but not for release groups (which have exactly the same issue, see https://musicbrainz.org/artist/7c7f9c94-dee8-4903-892b-6cf44652e2de) :) I personally liked it more before too (not sure extra space makes things look better) but I can see why consistent placement is good so not against the change if it feels like an improvement to others. |
I've added it for release groups as well now, although I've had some issues testing it with my local instance because it seems like the test data set doesn't have any cover art on release groups that I can find.
I can understand this. I would have liked not to have the space if there was no cover art at all, but that's a much more complex change. As is, I think it's a small but definite improvement. Another possibility is adding a different icon - perhaps a red picture with a line through it - to signify missing cover art explicitly. This could link with MBS-5450?
Thanks for the code suggestion - I've made a slightly different change so that the blank space isn't added even when the release group entity has cover art.
I've re-shuffled the conditionals to avoid some repetition, thank you! I've also tested it on label pages now ( |
There was a problem hiding this comment.
Thank you!
- Code changes look good (with hidden whitespace change).
- New display looks good (with artist, label, release group pages).
Last details to update otherwise:
- The two commits should be squashed together and the resulting commit should be provided with a good description (more details) (to follow our GitHub guidelines).
- The pull request’s title and description should be updated accordingly.
- The linked ticket’s summary and description should be updated too.
Note: In any case, this patch won’t be merged until next Tuesday due to the upcoming database schema change which is scheduled for release on Monday.
|
Actually there seems to be a small glitch when there is a data quality icon for some releases and not some others. That wasn’t anticipated in the original ticket. See |
|
Thank you for the feedback!
I didn't notice this, thank you! I'm reluctant to add even more white space, so I tried moving the indicator to the right, which seems to work better.
|
There was a problem hiding this comment.
I tried moving the indicator to the right, which seems to work better.
Much better indeed! ✨
Thank you for this additional improvement and for the screenshot.
Please look into the latest above-mentioned details. It could now be something like “Align release titles despite indicators for cover art and data quality” (then just MBS-13263: Align release titles despite indicators for the commit subject line).
This aligns release titles even where there are cover art or data quality indicators. It does this by moving data quality indicators to the right of the release title and adding whitespace when there is a missing cover art indicator.
Thank you!
I've squashed the commits now, with an appropriate commit message. I'll update the PR and ticket! |
JadedBlueEyes
changed the title
JadedBlueEyes
changed the title
There was a problem hiding this comment.
Selenium failure can be safely ignored as it is random and unrelated to this PR; The Selenium tests did pass in build 10851 that was before the last forced push that changed nothing. 🚢
|
Congratulations on your first merged pull request for MusicBrainz Server! 🎉 |
|
Tested locally again, seems good :) Thanks! |
* master: Update POT files using the production database MBS-13577 / MBS-13578: Release filtering for missing data (#3262) MBS-13589: Handle Operabase /o links (#3268) Convert /static/scripts/release templates to use Flow component syntax Convert /static/scripts/relationship-editor templates to use Flow component syntax Convert some more /static/scripts/ templates to use Flow component syntax Remove seemingly useless double export Convert /static/scripts/edit/components templates to use Flow component syntax Convert /static/scripts/common/components templates to use Flow component syntax Drop dead code in AddEntityDialog Drop unneeded instance typing in hydrate input Convert /components templates to use Flow component syntax Convert /components/list templates to use Flow component syntax Convert /components/Aliases templates to use Flow component syntax Convert /layout sidebar templates to use Flow component syntax Convert /layout templates to use Flow component syntax MBS-13585: Report for non-video recordings with video relationships (#3266) MBS-13581: Check "renamed into" in ArtistsThatMayBeGroups (#3265) Convert /edit/details templates to use Flow component syntax Convert /edit/details/historic templates to use Flow component syntax Convert /edit/component templates to use Flow component syntax Convert /edit templates to use Flow component syntax Update React to 18.3.1 (#3249) MBS-13263: Align release titles despite indicators (#3256) MBS-13588: Relationship phrase groups with pending edits are sometimes not highlighted Add `PendingEditsRoleT` to `RelatableEntityRoleT` Make `Entity::Artist::TO_JSON` consistent with `ArtistT` Use consistent chained method formatting Cache pending requests for `getOrFetchRecentItems` MBS-12741: Convert Create/Edit attribute admin pages to React Fix typo Limit `getReleaseGroupRecordings` to 5 attempts MBS-13558: Convert /report templates to use Flow component syntax MBS-13558: Convert /report/component templates to use Flow component syntax MBS-13573: Support a second format of Ticketmaster links MBS-13572: Support LiveNation links with _ and - MBS-13559: Don't reject dailysomething Bandcamp pages (#3255) Add context to newly added [removed] (#3246) Reuse same 'view list' string everywhere (#3247) Document that language packs must be installed Fix installing translations for beta/test websites admin/cron/hourly-sitemaps.sh: Dump FKs from READONLY Add missing scripts required by JSON dumps/sitemaps
Problem
This fixes MBS-13263 - that releases without cover art were misaligned in release group listings.
Solution
This adds a blank space the size of the icon before releases without cover art, to align them properly. To do that, it also adds a
blank-iconclass. It also moves data quality indicators to the rightTesting
I visually inspected the changes at
/release-group/6c08878c-d6a1-37b6-84c3-9566748545c1,/release-group/16a9cef3-b470-3ae1-bf10-cc04232d3e21and/artist/c0a1179b-b14a-4d68-a3c1-1fdab16ed602/releasesusing the sample database.Documenting
These changes should not impact the documentation.
Further action