feat(settings): Turn the theme toggle into a field in the settings form. #184
Conversation
WalkthroughThe changes update the settings dialog interface by replacing the previous theme selection component with a new one. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SD as SettingsDialog
participant TSF as ThemeSwitchFormField
participant MUI as useColorScheme Hook
U->>SD: Open settings dialog
SD->>TSF: Render theme switch component
TSF->>MUI: Retrieve current theme mode
U->>TSF: Select theme mode (Light/Dark/System)
TSF->>MUI: Invoke setMode() with selected mode
MUI-->>TSF: Return updated theme mode
TSF->>U: Display current theme in helper text
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@junhaoliao - is there a technical reason for this change? Or is it just aesthetic? |
mostly aesthetic. We need to find a place for the button group when we move the setting options into a tab. |
|
what do you mean by this |
The (toggle) buttons might not be suitable to show up as tab |
|
Ah I get it now. Is there a reason you dropped the |
One of the reasons is to make the button group narrower to avoid x-direction overflow issues in the tab. another reason is to make the purpose of the field, which is to "override a theme mode", more explicit. this can be subjective lol. feel free to object |
|
Yes it is subjective. I think it's better with the system (as without helper) , as long as it fits in the default tab width. I think the double click is slightly less intuitive, to go back to system I would rename the title to |
junhaoliao
changed the title
junhaoliao
requested review from
davemarco
and removed request for
davemarco
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/modals/SettingsModal/index.css (1)
7-7: Consider alternative to !important.The use of !important should be avoided as it makes styles harder to maintain. Consider increasing selector specificity instead.
- font-size: 2rem !important; + font-size: 2rem;If you need to override MUI styles, consider using a more specific selector:
.MuiDialog-root .settings-dialog-title { font-size: 2rem; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/modals/SettingsModal/SettingsDialog.tsx(2 hunks)src/components/modals/SettingsModal/ThemeSwitchFormField.tsx(1 hunks)src/components/modals/SettingsModal/ThemeSwitchToggle.tsx(0 hunks)src/components/modals/SettingsModal/index.css(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/modals/SettingsModal/ThemeSwitchToggle.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/modals/SettingsModal/ThemeSwitchFormField.tsxsrc/components/modals/SettingsModal/SettingsDialog.tsx
🔇 Additional comments (3)
src/components/modals/SettingsModal/ThemeSwitchFormField.tsx (2)
1-15: LGTM! Well-organized imports.The imports are logically grouped and all are utilized in the component.
23-62: Consider PR discussion points about theme options.Based on the PR discussion:
- The "System" option was suggested to be retained for better user experience
- The title could be simplified to just "Theme" (already implemented)
- The current mode display might be redundant
Would you like to:
- Keep the current implementation with the "System" option as suggested in the discussion?
- Remove the FormHelperText displaying the current mode if it's deemed redundant?
src/components/modals/SettingsModal/SettingsDialog.tsx (1)
32-32: LGTM! Clean integration of the new theme switch component.The ThemeSwitchFormField is well-placed within the DialogContent, providing a more consistent form layout.
Also applies to: 144-144, 147-147
| <FormHelperText> | ||
| {`Current mode: ${mode}`} | ||
| </FormHelperText> |
There was a problem hiding this comment.
| <FormHelperText> | |
| {`Current mode: ${mode}`} | |
| </FormHelperText> |
There was a problem hiding this comment.
For visual consistency with other fields, how about:
| <FormHelperText> | |
| {`Current mode: ${mode}`} | |
| </FormHelperText> | |
| <FormHelperText> | |
| {"When \"System\" mode is selected, the theme adapts to your operating system or" + | |
| " individual user agent's preference."} | |
| </FormHelperText> |
There was a problem hiding this comment.
Maybe something simpler. Log viewer color theme. System theme will match your system settings.
| import { | ||
| Button, | ||
| FormControl, | ||
| FormHelperText, |
There was a problem hiding this comment.
Related to later comment
| FormHelperText, |
There was a problem hiding this comment.
Believe there is an extra space above font-size that's not neccesary
There was a problem hiding this comment.
hmm.. Do you mean an extra line above font-size? That was added by the linter (stylelint-config-clean-order)
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit