-
Notifications
You must be signed in to change notification settings - Fork 57
Indicate to AT that element is currently selected #1408
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
Conversation
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.
Nice!
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.
Good change! But one small problem
lib/components/viewers/route-row.tsx
Outdated
( | ||
<FormattedMessage id="components.RouteViewer.currentlySelected" /> | ||
) |
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.
We've been told in the past that brackets have to be localized.
I updated this PR to also add this string to the locale selector and the sorting dropdown! @miles-grant-ibigroup @daniel-heppner-ibigroup |
…opentripplanner/otp-react-redux into route-viewer-currently-selected
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 good! Just a few small issues
@@ -192,6 +192,11 @@ export default function NarrativeItinerariesHeader({ | |||
role="option" | |||
> | |||
{sortOption.text} | |||
{sortText === sortOption.text && ( | |||
<InvisibleA11yLabel> | |||
<FormattedMessage id="coommon.currentlySelected" /> |
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.
Typo!
i18n/fr.yml
Outdated
@@ -76,6 +76,7 @@ actions: | |||
réessayez. | |||
common: | |||
coordinates: "{lat}; {lon}" | |||
currentlySelected: (actuellement sélectionné) |
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.
Do we need a hardcoded space here to prevent the words from squishing together?
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.
might as well!
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.
you 🤝 codespell
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.
One more typo and then this great fix is ready to go!
@@ -192,6 +192,11 @@ export default function NarrativeItinerariesHeader({ | |||
role="option" | |||
> | |||
{sortOption.text} | |||
{sortText === sortOption.text && ( | |||
<InvisibleA11yLabel> | |||
<FormattedMessage id="commmon.currentlySelected" /> |
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.
Oh no!
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.
cant spell this to save my life huh
Description:
Adds a "(currently selected)" string to active links so AT users know which route is active.
Also adds it to locale selector and sort option since
aria-selected
is being a little inconsistent on those components.PR Checklist: