-
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 configuration types. #1017
Add configuration types. #1017
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.
I think this looks good. Grouping the types in a consistent way and in one file is a good idea. I just had a few questions:
@@ -89,7 +90,7 @@ class AppMenu extends Component< | |||
|
|||
_triggerPopup = () => { | |||
const { popupTarget, setPopupContent } = this.props | |||
setPopupContent(popupTarget) | |||
if (popupTarget) setPopupContent(popupTarget) |
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.
Why is this change added?
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.
popupTarget
is optional, so TypeScript forces this truthy check before triggering the popup.
const modeColors: ModeColors = {} | ||
transitModes.forEach((mode: ConfiguredTransitMode) => { | ||
state.otp.config.modes.transitModes.forEach((mode) => { |
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 you want to also add the
const { config: otpConfig } = state.otp
here?
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.
Not needed, in this file, it is used only on this line.
@@ -95,10 +95,3 @@ export type SetViewedRouteHandler = (route?: ViewedRouteState) => void | |||
export type SetViewedStopHandler = (payload: { stopId: string } | null) => void | |||
|
|||
export type SetLocationHandler = (payload: MapLocationActionArg) => void | |||
|
|||
export interface ConfiguredTransitMode { |
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.
Why is this interface being removed?
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 moved it to config-types.ts
, and it is now called TransitModeConfig
.
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 is some great cleanup, thanks for all your work!
# Allows calorie counts to be hidden | ||
displayCalories: true |
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.
Was this behavior removed?
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.
Displaying calories was completely removed from OTP-UI, so this doesn't apply anymore.
displayCalories: | ||
typeof itinerary?.displayCalories === 'boolean' |
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.
Why was this removed?
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.
Displaying calories was completely removed from OTP-UI, so this doesn't apply anymore.
Description
This PR introduces types used in the app configuration (config.yml), and a partial redux state type is added.
The new types are not necessarily exhaustive and may not include obsolete config properties still in example-config.yml.
A few component files have been changed just to give an idea of the direction taken.
Note that calltaker config types are still to be implemented.
PR Checklist