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 Configure Screens page #249

Merged
merged 163 commits into from
Jan 24, 2024

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Jan 10, 2024

Asana task: [Perm config] Configure individual screens

Added the last page of the workflow for configuring individual screens. Any existing screens will appear as a row when navigating to the page. Any new/updated row is added to state. This state will be used in the next task for posting to S3.

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 great overall, just some little things around unintentional mutation on the TS side + tacking extra fields onto a struct on the elixir side.

I'll be quick with the followup review, promise! 🏃‍♂️

lib/screenplay/jason_screen_encoder.ex Outdated Show resolved Hide resolved
assets/js/utils/api.ts Outdated Show resolved Hide resolved
lib/screenplay/pending_screens_config/cache.ex Outdated Show resolved Hide resolved
lib/screenplay/screens_config/cache.ex Outdated Show resolved Hide resolved
lib/screenplay_web/controllers/config_controller.ex Outdated Show resolved Hide resolved
lib/screenplay_web/controllers/config_controller.ex Outdated Show resolved Hide resolved
lib/screenplay_web/controllers/config_controller.ex Outdated Show resolved Hide resolved
lib/screenplay_web/controllers/config_controller.ex Outdated Show resolved Hide resolved
@cmaddox5 cmaddox5 assigned jzimbel-mbta and unassigned cmaddox5 Jan 23, 2024
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.

A few more little things but otherwise good to go! 🐬

});
}, [existingPendingScreens, newScreens]);

const getExistingLiveScreens = () => existingScreens?.live_screens ?? {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just assign the existing live screens to a variable here, instead of making a function to get them? The function gets called multiple times below, including in a loop, which means this work (while very negligible) gets unnecessarily repeated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, for whatever reason I only tried assigning it to a variable in the hook. If done that way, the variable never updates in time for a render. Assigning it here like you suggested does work, so yes I will go with that. Thanks!

Comment on lines 185 to 186
{Object.keys(getExistingLiveScreens()).map((screenID) => {
const screen = getExistingLiveScreens()[screenID];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Object.entries to get both the ID and the config simultaneously, if you like:

Object.entries(existingLiveScreens).map(([screenID, screen]) => {...})

setSelectedPlaces(existingSelectedPlaces.add(place.id));
const placeToAdd = places.find((place) => place.id === item.id);
if (placeToAdd) {
setSelectedPlaces(existingSelectedPlaces.add(placeToAdd.id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar issue here, the new state value is "equal" to the previous one since both are references to the same Set object, which you mutate to add the new ID.

To make sure react sees that there was a change, you can make a copy for the new state value like so:

Suggested change
setSelectedPlaces(existingSelectedPlaces.add(placeToAdd.id));
setSelectedPlaces((prev) => new Set([placeToAdd.id, ...prev]));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did handle that issue with the variable existingSelectedPlaces on the line above. That said, I think I do like the extra simplicity of making a new Set in the state setter.

@cmaddox5 cmaddox5 merged commit f108ccb into permanent-configuration Jan 24, 2024
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/gl-eink-configure-screens-page branch January 24, 2024 18:38
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