ticket-charts: add smooth mode switch #1678
Conversation
| <li class="nav-item active"> | ||
| <a | ||
| class="nav-link active" | ||
| href="javascript:void(0);" |
There was a problem hiding this comment.
PR looks good, thanks.
@buck54321 I've wondered about the use of an anchor with the href javascript hack instead of a button. Thoughts about changing this?
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#Accessibility
Anchor tags are often abused as fake buttons by setting their href to "#" or "javascript:void(0)" to prevent the page from refreshing, then listening for their click events .
These bogus href values cause unexpected behavior when copying/dragging links, opening links in a new tab/window, bookmarking, or when JavaScript is loading, errors, or is disabled. They also convey incorrect semantics to assistive technologies, like screen readers.
Use a
<button>instead. In general, you should only use a hyperlink for navigation to a real URL.
But the inline javascript is what stands out to me as a potential challenge for deploying a good CSP.
There was a problem hiding this comment.
I don't really understand why these are links at all. We should be applying the styling directly to the <li> elements, and making those elements the stimulus targets. This seems to be based on a pattern from the bootstrap docs. Interestingly, in the docs, they do use the anchor tag for the href.
@njirap, use href="#" for now in this PR. Another job might be to go through and remove all the links.
There was a problem hiding this comment.
@buck54321 something's broken with the href="#" looking into it
There was a problem hiding this comment.
Maybe with onclick? https://stackoverflow.com/a/1291944
If that doesn't work, we can stick with the javascript: until we can entirely nuke these anchors.
There was a problem hiding this comment.
Using anchor tags is just an old convention since they automatically deal with small things like styling cursor hover and active state in a way that feels native to whatever client the end user is on.
Also if the application is using url backed state then its kind of semantic to use an anchor tag since the state change of clicking on it ( eg a query param to represent line mode) would get reflected in the url updating as well.
So I tend to just stick to using empty <a href=""> tags out of habit and simplicity.
But I think just using a utility class like .clickable { cursor: pointer; } and using any element that seems most semantic to handle clicks is just fine as well!
Just sharing my 2 cents here since I may have played a role in seeding this pattern in this repo. Don't have any qualms about moving away from the pattern if it feels wrong to folks.
There was a problem hiding this comment.
Also if the application is using url backed state then its kind of semantic to use an anchor tag since the state change of clicking on it ( eg a query param to represent line mode) would get reflected in the url updating as well.
That's a good point about updating the URL, but I wouldn't find it odd for a button to do the same.
There was a problem hiding this comment.
Now tracking this in issue #1679
There was a problem hiding this comment.
OK, we'll save the anchor javascript issue for another day. Anything else to add for this PR @buck54321 ?
views/charts.tmpl
Outdated
| <li class="nav-item"> | ||
| <a | ||
| class="nav-link" | ||
| href="#" |
There was a problem hiding this comment.
Please revert this one back too.
fixes #1502