allow multiple transport modes to be specified in a single transportmode #1282
Conversation
daniel-heppner-ibigroup
force-pushed
the
allow-multiple-transport-modes
branch
from
cd81a2f to
6c100ce
Compare
| - mode: ["TROLLEYBUS", "BUS"] | ||
| # When mode is an array an overrideMode must be provided. | ||
| # This specifies the displayed mode used for icon and URL param. | ||
| overrideMode: BUS |
There was a problem hiding this comment.
This key title is confusing. But I can't think of a better one
There was a problem hiding this comment.
Was trying not to make it a breaking change.
There was a problem hiding this comment.
we could've done a thing where we just use the first item in the array as the override but this is fine too!
There was a problem hiding this comment.
that's not a bad idea! we'll see what second reviewer thinks
There was a problem hiding this comment.
The name can be confusing at first. Maybe change overrideMode to summaryMode or displayedMode but I don't feel strongly about it. It looks like branded names are already supported, so that should be fine.
There was a problem hiding this comment.
I wanted to keep it so we didn't change the config at all. I agree that the name choice wasn't great but I don't think it's worth updating now.
| @@ -135,10 +135,18 @@ export function getInitialState(userDefinedConfig) { | |||
|
|
|||
| const transitModeSettings = config?.modes?.transitModes.map((transitMode) => { | |||
| const { mode, overrideMode } = transitMode | |||
| if (Array.isArray(mode) && !overrideMode) { | |||
| console.warn( | |||
There was a problem hiding this comment.
When this warning fires the entire app does still crash but I guess the warning is there so that's good enough
miles-grant-ibigroup
assigned daniel-heppner-ibigroup and unassigned miles-grant-ibigroup
daniel-heppner-ibigroup
assigned binh-dam-ibigroup and unassigned daniel-heppner-ibigroup
| - mode: ["TROLLEYBUS", "BUS"] | ||
| # When mode is an array an overrideMode must be provided. | ||
| # This specifies the displayed mode used for icon and URL param. | ||
| overrideMode: BUS |
There was a problem hiding this comment.
The name can be confusing at first. Maybe change overrideMode to summaryMode or displayedMode but I don't feel strongly about it. It looks like branded names are already supported, so that should be fine.
Description:
We need to allow several modes to be grouped together under a single icon. This will allow combining rail modes (like TRAM, RAIL, SUBWAY) together or bus modes (BUS, TROLLEYBUS) together under a single checkbox.
Support for this already existed in OTP UI so we are just enabling support for it in OTP RR with this change.