-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix keyboard shortcuts snackbar text showing stale values #3165
Fix keyboard shortcuts snackbar text showing stale values #3165
Conversation
This pull request is being automatically deployed with Vercel (learn more). nusmods-website – ./website🔍 Inspect: https://vercel.com/nusmodifications/nusmods-website/pn5cgya4f nusmods-export – ./export🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/gs6edg29b |
Codecov Report
@@ Coverage Diff @@
## master #3165 +/- ##
==========================================
- Coverage 55.44% 55.43% -0.02%
==========================================
Files 253 253
Lines 5312 5313 +1
Branches 1226 1226
==========================================
Hits 2945 2945
- Misses 2367 2368 +1
Continue to review full report at Codecov.
|
Deployment preview for |
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.
Thanks @chrisgzf for fixing a bug I caused 😆
@@ -33,8 +33,7 @@ const KeyboardShortcuts: React.FC = () => { | |||
const [helpShown, setHelpShown] = useState(false); | |||
const closeModal = useCallback(() => setHelpShown(false), []); | |||
|
|||
const mode = useSelector(({ settings }: State) => settings.mode); | |||
const themeId = useSelector(({ theme }: State) => theme.id); | |||
const store = useStore(); |
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 it possible to use this instead so that we don't have to keep casting the store type?
const store = useStore(); | |
const store = useStore<State>(); |
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.
Woops, forgot that this exists. Fixed in c282a2d.
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.
Thank you!
In #3007 there was a regression when we functionalized
KeyboardShortcuts
. Whenever we press x to toggle light/dark mode or z/c to cycle themes, the snackbar that pops out will always show the old value. This can be repro-ed in prod.This is due to our useSelector hook always capturing the old stale value when displaying the notification snackbar, due to the rendering lifecycle of functional components.
To fix this, we directly access the redux store when we want to display the notification snackbars, to get the most current values.