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-8820: Show explanation if relationships tab shows no rels #1977

Merged
merged 1 commit into from Sep 13, 2021

Conversation

reosarevok
Copy link
Member

Fix MBS-8820

For an artist that only has event relationships (or, I assume, a label that only has relationships of a type that gets filtered out by groupRelationships), the relationships tab is currently just empty.
This changes it so that Relationships displays an informative message if all existing relationships have been filtered out. While at it, I also got rid of the separate "no rels" message on ArtistRelationships and LabelRelationships, and moved that to Relationships as well.

Tested locally by creating the appropriate situations (artist without any rels, artist with only event rels) and navigating around to see that the changes didn't seem to affect other relationship displays.

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Mar 12, 2021
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.

LGTM but did not test.

Comment on lines 115 to 119
{exp.l(
'{link} only has relationships that are displayed elsewhere.',
{link: <EntityLink entity={source} />},
)}
Copy link
Member

Choose a reason for hiding this comment

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

I do wish we could make this a bit less vague by saying where "elsewhere" is. As @yvanzo suggested in the meeting notes:

Maybe we should just leave a note about event and work relationships to the user when the relationships tab is empty.

Having a custom string for each entity type doesn't seem ideal, but maybe a list of links to the other tabs would work? I'm not sure what's best here. Do you think this string is the best we can do until we can make more substantive UI improvements? If so I'm okay with merging it as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure how to do this otherwise. I guess we could check every time whether one or more event rels exist. Work rels are actually displayed, just under the appearances table further below, same as recording rels, for example (which are also filtered out).

Copy link
Member

@mwiencek mwiencek Mar 24, 2021

Choose a reason for hiding this comment

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

Maybe separate strings wouldn't be so bad then. Since artists and labels are the only entities with dedicated relationship tabs, the only two places you'd have to tell the user to check are (1) the events tabs for artists or (2) the index page for labels, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, actually I'm not even sure whether there's currently any rel for labels that does not appear on the relationships tab, either as in the relationship list or the table. So we probably only need this for artist, heh. Changing the string to be artist-specific (and the section to check if the entity is an artist), but we can change it to a string selector if you can think of any other use cases that need it.

@yvanzo yvanzo added this to the 2021-09-20 milestone Sep 7, 2021
For an artist that only has event relationships, the relationships tab
is currently just empty.
This changes it so that Relationships displays an informative message
if all existing relationships have been filtered out.
While at it, I also got rid of the separate "no rels" message
on ArtistRelationships and LabelRelationships, and moved that
to Relationships as well.
@reosarevok reosarevok merged commit e380d64 into metabrainz:master Sep 13, 2021
@reosarevok reosarevok deleted the MBS-8820 branch September 13, 2021 18:19
mwiencek added a commit that referenced this pull request Sep 13, 2021
* master:
  Update POT files using the production database
  Replace "special entity" with "special purpose entity"
  Update translations from Transifex
  MBS-11130: Clarify it's not possible to subscribe to VA / [no label] (#1754)
  MBS-11841: Convert Set Track Lengths edit to React (#2234)
  MBS-11819: Show to admins whether account has oAuth uses (#2250)
  MBS-8820: Show explanation if artist relationships tab shows no rels (#1977)
  MBS-8098: Allow release-group-level-rels in release lookup (#2213)
  MBS-6140: Allow (recording|work)-level-rels for release browse (#2173)
  MBS-11940: Properly error for invalid XML sent to /ws/2/tag (#2258)
  MBS-11843: Add report for "Events with annotations" (#2193)
  Make user/tag pages 403 if tag is meant to be private
  Add test for RemoveEmpty url
  Make the timeline icon clearer & easier to click
  MBS-11794: Link to edit searches on statistics/edits page
  MBS-11799: Display "credited as" field in sidebar external links (#2180)
  Ensure tooltip content has standard formatting
  MBS-11717: Also show edits pending in ArtistCreditUsageLink
  MBS-11944: Fix label on button for place merges (#2254)
  MBS-11907: Block vyd.co link aggregator (#2249)
  In-code doc: Comment URL rel. UUIDs with name
  Bump Flow to 0.159.0
  MBS-11932: Also exclude work licenses from LinksWithMultipleEntities
  Set hostname of MB data export
  MBS-9454: Autoremove unused urls with daily.sh
reosarevok added a commit that referenced this pull request Sep 20, 2021
* beta:
  Update POT files using the production database
  Update translations from Transifex
  Update POT files using the production database
  Replace "special entity" with "special purpose entity"
  Update translations from Transifex
  MBS-11130: Clarify it's not possible to subscribe to VA / [no label] (#1754)
  MBS-11841: Convert Set Track Lengths edit to React (#2234)
  MBS-11819: Show to admins whether account has oAuth uses (#2250)
  MBS-8820: Show explanation if artist relationships tab shows no rels (#1977)
  MBS-8098: Allow release-group-level-rels in release lookup (#2213)
  MBS-6140: Allow (recording|work)-level-rels for release browse (#2173)
  MBS-11940: Properly error for invalid XML sent to /ws/2/tag (#2258)
  MBS-11843: Add report for "Events with annotations" (#2193)
  Make user/tag pages 403 if tag is meant to be private
  Add test for RemoveEmpty url
  Make the timeline icon clearer & easier to click
  MBS-11794: Link to edit searches on statistics/edits page
  MBS-11799: Display "credited as" field in sidebar external links (#2180)
  Ensure tooltip content has standard formatting
  MBS-11717: Also show edits pending in ArtistCreditUsageLink
  MBS-11944: Fix label on button for place merges (#2254)
  MBS-11907: Block vyd.co link aggregator (#2249)
  In-code doc: Comment URL rel. UUIDs with name
  Bump Flow to 0.159.0
  MBS-11932: Also exclude work licenses from LinksWithMultipleEntities
  Set hostname of MB data export
  MBS-9454: Autoremove unused urls with daily.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
3 participants