-
-
Notifications
You must be signed in to change notification settings - Fork 286
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-13804: Show review links as such on sidebar #3404
base: master
Are you sure you want to change the base?
Conversation
It of course doesn't have to be that icon set - if you want to design some MeB icons for that, or find some other free use ones, that's more than ok :) I kind of agree that the current blog one is a good fit for review, just not sure if it would confuse users too much to switch it. |
We discussed on IRC and agreed the change is probably minor enough, so I made it now. |
The previous blog icon will be reused for review links in following commits. I updated the source for our Gentleface Mono Icon Set icons since gentleface.com itself seems gone.
Similarly as with blog and homepage, this shows all review rels as such regardless of what the URL is, rather than showing, say, YouTube links as "Play on YouTube".
On the sidebar, we only show the default per-site icon if none of the special icons apply. On the editing page, we were doing the other way around. Consolidating it seems sensible and if so we should do the special icons first, since overriding per-page ones is part of their whole point.
a82ec58
to
8c74e28
Compare
While I'm not sure if this is faster than three nested .some calls, it shouldn't be a lot slower and I'd say it makes the code at least somewhat more clear. This does no longer prioritize blog over review, but that probably doesn't matter since I'm not sure those can exist at the same time for the same entity type and even if they could, something being a blog and review for the same entity at the same time would just be bad data, so it's irrelevant which icon it shows.
These names are just "blog" and "review".
8c74e28
to
9bdf62d
Compare
Thanks, the new code looks a lot better. I feel like the display can be improved, though: It'd be nice to display the short name of the domain hosting the review, like:
That way you know where the review links go without having to awkwardly hover over them. |
Added a commit implementing showing the hostname :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great but please remove the console.log
before merging. :)
@@ -87,6 +87,7 @@ component ExternalLinks( | |||
for (let i = 0; i < relationships.length; i++) { | |||
const relationship = relationships[i]; | |||
const target = relationship.target; | |||
console.log(target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(target); |
For reviews, it can be annoying to have just a "Review" link with no info about what kind of review it is and have to hover in order to find out. This shows the hostname of the review URL on the sidebar, so that the user can decide based on that what review(s) they are interested in. I remove www. because it doesn't seem to add anything of value and takes sidebar space.
Implement MBS-13804
Problem
Reviews are shown on the sidebar of entities as whatever matches their URL, which can be very confusing. For example, a YouTube video review on the sidebar of a release group will say "Play on YouTube", but clearly it's not a link to play the RG.
Solution
Similarly as with blog and homepage, this shows all review rels as such regardless of what the URL is, rather than showing the default for the URL. The second commit fixes the order we look for favicons also on the edit pages.
I updated the source for our Gentleface Mono Icon Set icons since gentleface.com itself seems gone.
Testing
Manually, using the RG from the ticket,
release-group/a2105081-b86f-4b56-a9cc-ddda4835b975
, with full server data.