LB-947: Change entity pills on charts page from button to anchor #2672
Conversation
| @@ -239,7 +244,7 @@ export default class UserEntityChart extends React.Component< | |||
| count: elem.listen_count, | |||
| caaID: elem.caa_id, | |||
| caaReleaseMBID: elem.caa_release_mbid, | |||
| artists: elem.artists | |||
| artists: elem.artists, | |||
There was a problem hiding this comment.
this change was made by prettier, looks like there are a few outstanding lint issues? the dev environment automatically runs prettier, so there's no good way to get around including this diff in the PR without messing with Git hunks
There was a problem hiding this comment.
Don't worry about these extra linting changes, indeed we merged some un-linted changes in master.
| onClick={(e) => { | ||
| // LB-925 ctrl-click and cmd-click shouldn't navigate in this tab | ||
| if (!e.ctrlKey) { | ||
| e.preventDefault(); | ||
| this.changeEntity("release-group"); | ||
| } |
There was a problem hiding this comment.
I think this mechanism should directly be implemented in the Pill component rather than only for this page, so that navigation is consistent across our different pages that use this Pill.
We would have something like this in Pill:
const {onClick} = props;
const customOnClick = (e) => {
if (!e.ctrlKey) {
e.preventDefault();
onClick();
}
}
…
<a onClick={customOnClick} …>
| @@ -239,7 +244,7 @@ export default class UserEntityChart extends React.Component< | |||
| count: elem.listen_count, | |||
| caaID: elem.caa_id, | |||
| caaReleaseMBID: elem.caa_release_mbid, | |||
| artists: elem.artists | |||
| artists: elem.artists, | |||
There was a problem hiding this comment.
Don't worry about these extra linting changes, indeed we merged some un-linted changes in master.
A good question. I think the answer for now is "don't worry about it", as we started talking about moving to a single page application which would change the way we handle navigation. |
|
I had to manually accept for our CI build to be run, and now that that is done there are a few failing tests regarding this change from button to anchor. Let me know if you need anything ! |
|
Hi @piperswe ! |
|
@piperswe Ping! |
Problem
https://tickets.metabrainz.org/browse/LB-947
Dipping my toes in, looking to contribute something to a project I've been loving lately!
Solution
I switched the element from a
<button>to an<a>, and also added href properties to their usage inUserEntityChart. It defaults to#as a hash to preserve the pointer cursor, and for accessibility reasons. Changed validated locally - URL appears in status bar and open-in-new-tab (both middle click and right click -> open in new tab) seems to work fine. I integrated the proposed fix in LB-925, so ctrl/cmd+click also work.Open question: should it only be an
<a>if the href property is set? Semantically,<button>makes sense in several of the other placesPillis used. Not sure how much it matters though.