-
-
Notifications
You must be signed in to change notification settings - Fork 71
fix(Toggle):Enhance theme toggle with improved icons and animations #801
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
base: main
Are you sure you want to change the base?
Conversation
- Add better sun and moon icons for light/dark modes - Implement custom dice icon for random theme with green color - Add shake animation effect for random theme icon - Ensure proper visibility of theme toggle elements in all modes - Improve accessibility with descriptive hover titles
Summary of ChangesHello @Anubhava2105, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the user experience of the theme toggle feature. It replaces generic icons with visually distinct sun, moon, and dice SVGs for dark, light, and random themes, respectively. The random theme now boasts a unique green color scheme and an engaging shake animation on hover. Furthermore, accessibility is improved by dynamically updating the toggle button's title attribute to inform users about the current theme and the next available option. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
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.
Code Review
This pull request does a great job of enhancing the theme toggle feature with new icons for each state and a fun shake animation for the random theme. The accessibility improvement of updating the button's title attribute is also a valuable addition. My review includes a few suggestions to refine the implementation. In the JavaScript file, there's an opportunity to reduce code duplication by creating a helper function for updating the theme button's title. In the CSS, some rules for styling the new random theme icon appear to be redundant and could be simplified, and an animation rule could be removed for a minor performance optimization.
frontend/assets/scripts/script.js
Outdated
| const themeLabels = { | ||
| dark: 'Currently in dark mode, click for light mode', | ||
| light: 'Currently in light mode, click for random mode', | ||
| random: 'Currently in random mode, click for dark mode', | ||
| }; | ||
| elmThemeToggleButton.setAttribute('title', themeLabels[currentTheme] || 'Toggle theme'); |
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.
This block of code, including the themeLabels object, is a duplicate of the logic at lines 315-320. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, you could refactor this.
I'd suggest defining the themeLabels object once at a higher scope (e.g., near your other constants) and creating a small helper function to set the title. You could then call this function both on initialization and here in the click handler.
Example refactoring:
// At a higher scope in the file
const themeLabels = {
dark: 'Currently in dark mode, click for light mode',
light: 'Currently in light mode, click for random mode',
random: 'Currently in random mode, click for dark mode',
};
function updateThemeButtonTitle() {
if (elmThemeToggleButton) {
elmThemeToggleButton.setAttribute('title', themeLabels[currentTheme] || 'Toggle theme');
}
}
// Then you can simply call `updateThemeButtonTitle()` here and on line 314.
frontend/assets/styles/custom.css
Outdated
| [data-theme='random'] .random-icon { | ||
| color: var(--toggle-icon-color); | ||
| } | ||
|
|
||
| [data-theme='random'] .random-icon rect { | ||
| fill: var(--toggle-icon-color); | ||
| stroke: var(--toggle-icon-color); | ||
| } | ||
|
|
||
| [data-theme='random'] .theme-toggle:hover .random-icon rect { | ||
| fill: var(--toggle-icon-hover); | ||
| stroke: var(--toggle-icon-hover); | ||
| } |
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.
These styling rules for the random icon and its rectangle appear to be redundant. The color property is already set on all theme toggle SVGs in the rule at lines 46-55, and the rect element inside the SVG uses currentColor for its fill and stroke, so it should inherit the color from its parent SVG automatically. This also applies to the :hover state.
Removing these rules would make the styling more consistent with the other icons (sun-icon, moon-icon) and simplify the CSS. The existing rules for .theme-toggle svg and .theme-toggle:hover svg should be sufficient.
frontend/assets/styles/custom.css
Outdated
| .theme-toggle:hover .random-icon { | ||
| animation: shake 0.8s ease; | ||
| } |
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.
This rule applies the shake animation to the .random-icon on hover, regardless of the current theme. When the theme is not 'random', this icon is invisible (opacity: 0), so the browser is doing unnecessary work animating an element that isn't visible. This can be a minor performance issue.
The rule at lines 131-133 already handles the animation when the random theme is active. You can safely remove this rule to avoid the unnecessary animation.
Made the changes given by gemini code assist to improve code maintainability
|
Hey, why are these checks failing? I don't think it has anything to do with the code. |
Those checks are just general failing tests we have. There are other open issues to fix those |
|
If any of the gemini conversations are completed you should click resolve |
Enhanced Theme Toggle with Improved Icons and Animations
Changes
#Screenshots


