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

Support larger custom route renderers #1079

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

miles-grant-ibigroup
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup commented Nov 27, 2023

Description:
Adjusts color handling to be more consistent and allow for more situations.

LOOKING FOR FEEDBACK:

  • things are not quite working: agency id is not showing up in the itinerary response, which is causing the incorrect color to appear in transitive

PR Checklist:

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

@miles-grant-ibigroup miles-grant-ibigroup added the WIP Work in progress label Nov 27, 2023
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

The route colors in transitive are working for me! Looks good

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Should we keep calculating a settings-based route color as fallback?

lib/components/viewers/RouteRow.js Outdated Show resolved Hide resolved
lib/components/viewers/viewers.css Outdated Show resolved Hide resolved
@@ -700,10 +700,6 @@ export const findRoute = (params) =>
newRoute.patterns = routePatterns
// TODO: avoid explicit behavior shift like this
newRoute.v2 = true
newRoute.color = getRouteColorBasedOnSettings(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep this as fallback if a route color is not provided in OTP, and for cases where we want to override route colors in config, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason it is removed here is that we instead do the override in every location where the color is used. This gives us more flexibility in how we use the color. Try it! a feed with no color still gets overriden

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

I think my questions have been answered.

@miles-grant-ibigroup miles-grant-ibigroup merged commit 1e49780 into dev Dec 12, 2023
9 checks passed
@miles-grant-ibigroup miles-grant-ibigroup deleted the support-larger-route-renderers branch December 12, 2023 14:41
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