Skip to content
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

Merged
merged 56 commits into from
Dec 20, 2023

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Dec 5, 2023

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).

@cmaddox5 cmaddox5 requested review from a team and jzimbel-mbta and removed request for a team December 5, 2023 21:17
@cmaddox5 cmaddox5 marked this pull request as ready for review December 5, 2023 21:17
@cmaddox5 cmaddox5 changed the title Cm/gl eink config page feat: GL E-Ink Config Workflow Dec 6, 2023
Copy link
Member

@jzimbel-mbta jzimbel-mbta left a 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)}
Copy link
Member

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.

Suggested change
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

Comment on lines 56 to 58
onChange={(e) => {
e.stopPropagation();
}}
Copy link
Member

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)

Copy link
Contributor Author

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))
Copy link
Member

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...)

Copy link
Contributor Author

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.

Comment on lines 208 to 216
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;
});
Copy link
Member

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!)

Copy link
Contributor Author

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.

Comment on lines 68 to 71
.h2 {
font-weight: 700;
font-size: 28px;
}
Copy link
Contributor

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

@cmaddox5 cmaddox5 deleted the branch permanent-configuration December 20, 2023 17:52
@cmaddox5 cmaddox5 closed this Dec 20, 2023
@cmaddox5 cmaddox5 reopened this Dec 20, 2023
@cmaddox5 cmaddox5 changed the base branch from cm/perm-config-select-screen-type to permanent-configuration December 20, 2023 18:01
@cmaddox5 cmaddox5 merged commit c9fe78a into permanent-configuration Dec 20, 2023
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/gl-eink-config-page branch April 9, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants