-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add Class Names #1236
Add Class Names #1236
Conversation
# Conflicts: # package.json # yarn.lock
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.
This looks pretty good but I have a few thoughts about redundant things. Also I'd like to be able to test it with the new ItinerarySupplement component
@@ -3,11 +3,12 @@ import { Itinerary, Leg } from '@opentripplanner/types' | |||
import coreUtils from '@opentripplanner/core-utils' | |||
import React from 'react' | |||
|
|||
export const getFirstTransitLeg = (itinerary: Itinerary): Leg | undefined => | |||
itinerary.legs?.find((leg: Leg) => leg?.from?.vertexType === 'TRANSIT') | |||
|
|||
export const getFirstTransitLegStop = ( |
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 even need this function? Could we just get rid of it? Where is it used?
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.
It's used in the metro itinerary!
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.
If it wasn't clear this comment is about getFirstTransitLegStop
. Literally all it does is return the stop from an itinerary. I don't know that we really need a whole function to do that. There's no logic there
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.
It's shorter to write out the method than to copy paste that line everywhere!
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.
I think this makes sense! Beautiful class names
# Conflicts: # __tests__/components/viewers/__snapshots__/nearby-view.js.snap # yarn.lock
Description:
Adds a bunch of class names all over the place to make post-compile styling easier.
PR Checklist: