feat: create branch picker element - #608
Conversation
- conditionally set colors for branches based on current state - refactor button rendering to reduce duplication
- include helper to determine branch for train pill - pass down branch picker selection setter prop - update tests to accommodate new prop
These items were not showing up properly after merging in main.
On mobile (specifically Safari on iOS) the address bar (which is now on the bottom of the screen) blocks the address picker. This resolves this issue. Of note, I setting `h-screen h-dvh` as Tailwind variables, but they seemed to conflict and it did not work.
| .app-wrapper { | ||
| height: 100vh; | ||
| height: 100dvh; | ||
| } |
There was a problem hiding this comment.
Highlighting my commit description: a9e3b0e
If there's a better approach here I'm open to it, but this seemed like the most direct way to ensure the branch picker was not blocked by mobile address bars.
| // update branch picker to reflect the branch the clicked train is on | ||
| const matchingStationList = stationLists.find((stations) => | ||
| stations.some((station) => | ||
| station.stop_ids.some( | ||
| (stopId) => stopId === match.vehiclePosition.stopId, | ||
| ), | ||
| ), | ||
| ); | ||
| if (matchingStationList) { | ||
| setBranchPickerSelection(branchForLadder(matchingStationList)); | ||
| } | ||
|
|
There was a problem hiding this comment.
question: I'm torn on this. spiritually it feels like we shouldn't have to search back through the stations to figure out what branch a train is on. is there a better way?
There was a problem hiding this comment.
Tried a different approach in 3b06ead.
This change sets the branch for each ladder on render, so we can avoid the lookup on each click.
(avoiding station lookup on each click)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note: experimented with using Copilot to resolve a merge conflict, which appeared to retain both my changes and the latest commits from main |
mathcolo
left a comment
mathcolo
left a comment
There was a problem hiding this comment.
I'm close to approving! just a few more spots
| const branchForLadder = (ladderConfig: LadderConfig): BranchPickerSelection => { | ||
| if (ladderConfig.some((s) => s.id === "place-asmnl")) return "Ashmont"; | ||
| if (ladderConfig.some((s) => s.id === "place-brntn")) return "Braintree"; | ||
| return "Alewife"; | ||
| }; | ||
|
|
There was a problem hiding this comment.
nit: could we make this route agnostic, too?
| @@ -0,0 +1,89 @@ | |||
| import { RouteId } from "../../models/common"; | |||
|
|
|||
| const branches = ["Alewife", "Ashmont", "Braintree"] as const; | |||
There was a problem hiding this comment.
nit: ugh, I hate to nit this, but maybe this could also be route agnostic?
| isActive: boolean; | ||
| onClick: () => void; | ||
| }) => { | ||
| const { bg, dotText } = branchColors.Red[branch]; |
There was a problem hiding this comment.
nit: Red hard-code- this should also probably take in a route argument?
| const activeBg: Record<BranchPickerSelection, string> = { | ||
| Alewife: | ||
| "bg-ladder-branch-picker-alewife-dot-dark light:bg-ladder-branch-picker-alewife-dot-light", | ||
| Ashmont: "bg-heavy-rail-ashmont", | ||
| Braintree: "bg-heavy-rail-braintree", | ||
| }; |
There was a problem hiding this comment.
observation: just observing that you route-agnosticked (sp? 😅) the implementation but not the tests. I'm okay with that if you are 👍
2 participants
Asana Task: 🪐 Create Orbit branch picker element
This adds a "branch picker" component to allow focusing of different branches. Right now, this stores the chosen branch in state, and adjusts the active/inactive display of the three branch buttons accordingly. Clicking a train pill will also update the branch selection to the branch containing that train. Light mode versions of colors are used where available.
Related work:
Note: focusing ladders on selection change and other scrolling behavior will be included in subsequent tasks
Alewife
Ashmont
Braintree
Checklist