-
Notifications
You must be signed in to change notification settings - Fork 6
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
Reset settings on poll close for VxScan #5462
Conversation
0f95c4b
to
df66d77
Compare
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.
LGTM!
setVoterLanguage(null); | ||
} | ||
}, | ||
}); |
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.
Sorry to come in with a late review after this was already merged - I only just saw it and was curious. In general, it's a best practice to put changes to frontend-state in the handler for the user action that triggered them. In this case, that would be the closePolls
function. useQueryChangeListener
is only intended to be used as a workaround for when we can't implement this pattern since the user action isn't triggered via the screen (e.g. if they are interacting with a different piece of hardware). I recommend this article from the React docs that digs into these concepts more.
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.
No worries, I appreciate it! Happy to make a subsequent PR. Thanks for explaining and for linking the article.
In this case, I was seeing that VoterSettingsManager
has the following comment: Side-effect component for monitoring for auth and voter session changes and resetting/restoring voter display settings and language choice as needed.
And it seems to be self-contained in managing settings, and I didn't see a streamlined way to update the settings within the PollWorkerScreen
component, so I assumed the side-effect reaction might be our expected approach. Anything in that thinking that you could help reframe?
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 think this is a limitation of the side-effect component idea. Now PollWorkerScreen
needs a way to access the voter settings state but can't get to it. The state needs to be lifted up to a common parent of whatever is consuming the voter settings state and PollWorkerScreen
.
A few ideas:
- Since there's already a
VotingSettingsManagerContext
, we could move the state to whatever component creates that context, and then expose some functions in the context to modify that state. I'd maybe model this as a stack, since the behavior we want is to be able to push and pop themes as users log in/log out. - We could move the state (and associated listeners) to
AppRoot
.
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.
Ah gotcha, so we are talking about making a slightly larger structural change. Those two approaches both make sense to me, I'll do some playing around and come up with a solution that feels fitting!
Overview
#5348
Demo Video or Screenshot
Old behavior: after polls close and poll worker card removal, old settings/themes persist
settings-dont-reset-old.mov
New behavior: after polls close and poll worker card removal, settings/themes reset
settings-reset-new.mov
Testing Plan
Checklist