-
Notifications
You must be signed in to change notification settings - Fork 535
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
Adds noTitle
option to RelativeTime component
#4635
Conversation
🦋 Changeset detectedLatest commit: 33f9c8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
noTitle
option to RelativeTime component
Integration failures are unrelated to this PR! |
Co-authored-by: Clay Miller <clay@smockle.com>
"name": "noTitle", | ||
"type": "boolean", | ||
"defaultValue": "", | ||
"description": "Removes the `title` attribute provided on the element by default" |
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.
Hi! 👋
For the docs, can you also add when is it recommended to skip title?
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.
In this description? Or are you talking about updating the in-progress docs PR?
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.
Ideally both places, but either is fine for now :)
Hi! Can i request additional context, what does passing Based on https://github.com/github/accessibility-audits/issues/5633, trying to understand if this should be the default behavior or only in specific cases? (which ones?) |
@siddharthkp Hi! Good question. Passing Ideally it'd be default behavior, but we'd need a larger conversation with stakeholders and design to come up with an accessible solution (we've attempted this in Office Hours, but there is nothing straightforward that we can do other than this without extensive testing). This is also to match the behavior introduced in PVC. |
Would it be preferable to always have the Just wanted to ask to see if this would be helpful or if not because of what you shared about usage at GitHub and how there isn't a good alternative yet when the title is not displayed 👀 |
Yes, it would be preferable to never have a title attribute! I added a story that shows how we can instead use a Tooltip on interactive elements, but that doesn't solve the case of non-interactive elements :/ If you think it's ok I am totally fine removing the |
Yes, this was very helpful! Please correct me if I understood this incorrectly:
(this is where I'm probably understanding incorrectly) To make both experiences similar, we need to replace title with an accessible element, like a tooltip. By removing
I see where you are coming from! I feel a little worried about How do you feel about primer components for RelativeTime (both PRC and PVC) using a tooltip internally instead of expecting it to be added in the product? It would lead to some inconsistency in the short term (between primer vs non-primer relative time), but hopefully will set the direction towards accessible + consistent |
@siddharthkp You are mostly correct with how you're understanding it! The other user we're excluding is keyboard users -- even if a user can navigate to the element because it's focusable, like a link, the title attribute does not display for them.
No, if we pass in the full date and time as the tooltip (check out the story Link with Tooltip) that shows the full date in the tooltip, but the shortened version as the main text.
Sure! We can think of this as a "baby step" to a fully accessible solution. Since we don't have one right now, this is better than what currently exists, but still not fully ideal. One example of where this can be used in the product is on PRs, where the timestamp acts as a link to that comment. We can use I am also thinking that if audit issues start coming in, similar to the one linked in the PR, it gives teams a way to fix the audit issue with minimal impact or needing to reassign the issue to Primer -- they have a few options of how to address it. They can do one of the following:
We considered this! The tricky part is that not all RelativeTime elements are interactive. Some are static text, which we can't render a Tooltip on in an accessible way. We considered having two components, RelativeTime (as-is) and RelativeTimeWithTooltip, but right now consumers wrap the RelativeTime component in a |
Ah, I seeee! Thanks you going into detail, now it's very clear to me.
I completely missed this detail! Summarising for myself first: Current scenario:
Ideal scenario:
|
@siddharthkp Everything you said looks good, one slight modification: "We add a title attribute to the element which has the full datetime, but is only available to visual users, not to screen reader users or keyboard or mobile users" And regarding:
Correct! Eventually we hope to have a better solution, but this is just one small step to getting there :) |
Very fair, happy to merge this as a temporary "unblocker" for teams.
Just wanted to note that this kind of change is scary because even though I called it a temporary unblocker, once it's in dotcom, it's not temporary anymore. To remove it from primer, we would need to create a plan to remove it in dotcom first. I'd nudge you to follow up with a discussion post where we can try to find a long term fix. |
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.
Approved. See also: #4635 (comment)
Ready to merge whenever you'd like
@siddharthkp Thank you! That totally makes sense. That plan would also need to include a deprecation plan for |
@siddharthkp posted! #4645 |
Blocked until a new release of https://github.com/github/relative-time-element ships and is updated in primer/react.
Closes https://github.com/github/accessibility-audits/issues/5633
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist