-
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 Configure Screens page #249
feat: GL E-Ink Configure Screens page #249
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 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! 🏃♂️
assets/js/components/Dashboard/PermanentConfiguration/Workflows/GlEink/ConfigureScreensPage.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Jon Zimbel <[email protected]>
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.
A few more little things but otherwise good to go! 🐬
}); | ||
}, [existingPendingScreens, newScreens]); | ||
|
||
const getExistingLiveScreens = () => existingScreens?.live_screens ?? {}; |
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.
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.
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.
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!
{Object.keys(getExistingLiveScreens()).map((screenID) => { | ||
const screen = getExistingLiveScreens()[screenID]; |
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.
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)); |
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.
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:
setSelectedPlaces(existingSelectedPlaces.add(placeToAdd.id)); | |
setSelectedPlaces((prev) => new Set([placeToAdd.id, ...prev])); |
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.
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.
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.