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

fix(ui): Remove theme classname on switch #81973

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Conversation

scttcper
Copy link
Member

removes the classname when toggling between themes

related #81611

@scttcper scttcper requested review from BYK and JonasBa December 11, 2024 18:58
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 11, 2024
@@ -28,6 +28,10 @@ const formGroups: JsonFormObject[] = [
{value: 'system', label: t('Default to system')},
],
getData: transformOptions,
onChange: () => {
// Remove any existing theme class from when the page was loaded
document.body.classList.remove('theme-light', 'theme-dark', 'theme-system');
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this a helper function so we don't repeat the theme class names in more than 1 place. That said, for the current state of things I'm fine with only 2 places and 3 names especially if it is unlikely that we'd need this anywhere else. Maybe add a TODO: note for this kind of future work?

@scttcper scttcper enabled auto-merge (squash) December 11, 2024 19:58
@scttcper scttcper merged commit ae3e08c into master Dec 11, 2024
43 checks passed
@scttcper scttcper deleted the scttcper/remove-body-theme branch December 11, 2024 20:02
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants