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

[system] Fix mode blink when open multiple sessions #33877

Merged
merged 10 commits into from Aug 24, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 9, 2022

Before

There is a flaw in using React.useEffect in the code which causes the mode to switch back and forth between sessions.

Screen.Recording.2565-08-15.at.12.47.33.mov

After

Screen.Recording.2565-08-15.at.12.53.10.mov

The tests already covered the common scenarios. This bug is a very edge case due to our documentation requirement.


@siriwatknp siriwatknp added bug 🐛 Something doesn't work package: system Specific to @mui/system labels Aug 9, 2022
@siriwatknp siriwatknp changed the title [system] Fix color scheme blink when open multiple sessions [system] Fix mode blink when open multiple sessions Aug 9, 2022
@mui-bot
Copy link

mui-bot commented Aug 9, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 8847f8f

@siriwatknp siriwatknp marked this pull request as ready for review August 15, 2022 07:17
@@ -611,7 +611,6 @@ describe('createCssVarsProvider', () => {
);

expect(screen.getByTestId('current-mode').textContent).to.equal('dark');
expect(global.localStorage.setItem.calledWith(customModeStorageKey, 'dark')).to.equal(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not necessary because the value is already dark.

return (
<CssVarsProvider modeStorageKey="dark-mode-specificity">
Copy link
Member Author

Choose a reason for hiding this comment

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

no need to use a custom key because the storage is cleared between tests in test/regressions/index.test.js

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2022
@siriwatknp siriwatknp merged commit 4fad139 into mui:master Aug 24, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants