Skip to content
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

Route renderer a11y #904

Merged
merged 18 commits into from
Jun 2, 2023
Merged

Route renderer a11y #904

merged 18 commits into from
Jun 2, 2023

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented May 19, 2023

Description:

Improves visual accessibility by requesting custom route renderers to fully render route names if needed (especially in the pattern viewer and stop viewer).

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • [na] Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all this cleanup! New structure makes sense, just one quick potential rename

@@ -25,21 +25,26 @@ const Block = styled.span<{ color: string; isOnColoredBackground?: boolean }>`
`}
`

type RouteRendererProps = {
export type RouteRendererProps = {
fullRender?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this to expandedRender or renderName?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped fullRender in acc109a. For immediate purposes the custom route renderer uses leg.onColoredBackground.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be tempted to refactor that leg.onColoredBackground TBH.

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable font size is looking a bit funky, otherwise this all looks good thank you for the cleanup

<RouteRenderer
isOnColoredBackground={route.operator?.colorMode?.includes('gtfs')}
// All GTFS bg colors look strange with the top border
leg={generateFakeLegForRouteRenderer(route, true)}
style={{
fontSize: routeNameFontSize(routeName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable font size might be causing problems...

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working for me thanks for the changes!

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route name layout looks really nice! LGTM

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route name layout looks really nice! LGTM

@binh-dam-ibigroup binh-dam-ibigroup merged commit f93c0a1 into dev Jun 2, 2023
@binh-dam-ibigroup binh-dam-ibigroup deleted the route-renderer-a11y branch June 2, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants