-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: GL E-Ink Config Workflow #241
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.
Looks good overall, just some minor things that should be quick to fix.
I'm approving now assuming you respond as you see fit, but feel free to assign it back to me after making changes if you want me to take another look.
<SortLabel | ||
label={SORT_LABELS["Green"][sortDirection]} | ||
sortDirection={sortDirection} | ||
onClick={() => setSortDirection((1 - sortDirection) as DirectionID)} |
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.
Looks like it's not 100% necessary here, but consider using the function updater setState form for this, since the new state depends on the value of the current state.
onClick={() => setSortDirection((1 - sortDirection) as DirectionID)} | |
onClick={() => setSortDirection((prevSD) => (1 - prevSD) as DirectionID)} |
https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state
onChange={(e) => { | ||
e.stopPropagation(); | ||
}} |
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.
Is there some kind of react-bootstrap magic that allows this event to still get picked up somewhere?
Oh, or is it that the click handler now goes higher up, on the div that contains this component? (Thus making this a controlled component)
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 actually isn't needed anymore. One of my earlier implementation had an issue with the default onChange
so I added this to keep it from doing anything. Now the onRowClick
handles all of the click logic for the row.
@@ -94,7 +94,7 @@ const PlaceRowAccordion: ComponentType<PlaceRowAccordionProps> = ({ | |||
place.screens.filter((screen) => !screen.hidden).length > 0; | |||
const rowOnClick = hasScreens | |||
? useAccordionButton(place.id, () => handleClickAccordion(place.id)) |
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.
Normally you're not supposed to conditionally call hooks, because react depends on their call order to identify them. We might run into weird bugs if the places-and-screens data updates and causes this place to now have screens when it previously did not (or vice versa) while this component is mounted.
It sounds a little weird, but could you check if it would work to consistently call useAccordionButton
, and only use its return value if the place has screens?
Like:
// Always call the `useAccordionButton` hook, but conditionally use its click handler. https://react.dev/learn#using-hooks
const handleAccordionClick = useAccordionButton(place.id, () => handleClickAccordion(place.id));
const rowOnClick = hasScreens ? handleAccordionClick : () => undefined;
(Side note, the new react documentation site really buries some critical info...)
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.
Very interesting. I feel like you mentioning this is jogging my memory a bit on this particular hook rule. That change actually works the same way, so I'll change it to play it safe.
assets/js/util.ts
Outdated
places.sort((placeA, placeB) => { | ||
const indexA = stationOrder.findIndex((station) => { | ||
return station.name.toLowerCase() === placeA.name.toLowerCase(); | ||
}); | ||
const indexB = stationOrder.findIndex((station) => { | ||
return station.name.toLowerCase() === placeB.name.toLowerCase(); | ||
}); | ||
return indexA - indexB; | ||
}); |
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 pretty inefficient since every time we need to compare two elements of places
(potentially once for every unique pair of elements—O(n^2)
), we need to iterate through stationOrder
twice. That's something like cubic time—O(m*n^2)
where m is stationOrder.length
and n is places.length
—in the worst case.
It should be faster to compute the indices once for each place, up front. You can also create a map from stationOrder
to make lookups faster. What we really want is something like Enum.sort_by/2
, but that method isn't available in standard JS so we can do it ourselves. Or, add lodash to the project and use its sortBy
function.
const stationOrderToIndex = Object.from_entries(
stationOrder.map((station, i) => [station.name.toLowerCase(), i])
);
const placesByStationOrder = places.map((place) => ({
place,
index: stationOrderToIndex[place.name.toLowerCase()]
}));
const sortedPlaces = placesByStationOrder
.sort(({index: i1}, {index: i2}) => i1 - i2)
.map(({place}) => place);
return reverse? sortedPlaces.reverse() : sortedPlaces;
(I didn't write this in vscode, so please double check that it works if you decide to use it!)
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.
Wow, great catch. This is working great as is so I'll stick with this instead of adding lodash
at the moment.
assets/css/screenplay.scss
Outdated
.h2 { | ||
font-weight: 700; | ||
font-size: 28px; | ||
} |
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.
just popping in to note that Mary says here H2 is actually larger
Asana task: Stub out page layout, with station selection list
This PR adds stubs out the first page of the GL E-Ink workflow. Includes the places list and state management to keep track of selected rows. Does not include the search box or the bottom action bar (both will be implemented in a separate task).