Added display mode selection feature and implemented compact mode - #34
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
/gemini review |
github-actions
Bot
added
improvement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable display mode feature. The implementation is well-structured, particularly the smooth transitions without DOM regeneration and the good use of accessibility attributes. I've identified a few areas for improvement, mainly related to adherence to the repository's TypeScript style guide regarding type safety. My comments provide specific suggestions to address these points.
We've added Default and Compact modes, allowing users to choose how status badges are displayed in the issue list. Key changes: - Added a Display Mode selection dropdown to the popup. - Compact mode: Displays only colored dots without text, and displays the full status name as a tooltip on hover. - Implemented smooth transitions by changing only classes and attributes without regenerating DOM elements when switching modes. - Added the data-status attribute to badges to maintain status information when switching modes. - Added accessibility attributes (role, aria-label, tabindex) to compact mode. fix #33
b25ed37 to
6c5ce27
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
Thank you for your contribution! This pull request adds a great new feature for customizing the display of status badges. The implementation is solid, especially the way you've handled mode switching without regenerating DOM elements. I've left a few comments with suggestions to improve maintainability, consistency, and performance by addressing some code duplication and minor inefficiencies. These are mostly related to adhering to the repository's style guide. Overall, great work!
| /* GitHub manual dark theme selection (data-color-mode="dark") */ | ||
| [data-color-mode="dark"] .project-status-badge { | ||
| background-color: rgba(110, 118, 129, 0.15); | ||
| color: #e6edf3; | ||
| border: 1px solid rgba(110, 118, 129, 0.3); | ||
| background-color: var(--badge-dark-bg); | ||
| color: var(--badge-dark-color); | ||
| border: 1px solid var(--badge-dark-border); | ||
| box-shadow: none; | ||
| } | ||
|
|
||
| [data-color-mode="dark"] .project-status-badge::before { | ||
| opacity: 0.85; | ||
| box-shadow: 0 0 0 1px rgba(13, 17, 23, 0.4); | ||
| box-shadow: var(--badge-dark-dot-shadow); | ||
| } | ||
|
|
||
| [data-color-mode="dark"] .project-status-badge--compact { | ||
| border-color: var(--badge-dark-compact-border); | ||
| box-shadow: var(--badge-dark-compact-shadow); | ||
| } | ||
|
|
||
| [data-color-mode="dark"] .project-status-badge--compact:hover { | ||
| box-shadow: var(--badge-dark-compact-hover-shadow); | ||
| } |
There was a problem hiding this comment.
The style rules for [data-color-mode="dark"] from line 119 to 138 are duplicates of the rules within the @media (prefers-color-scheme: dark) block (lines 95-116). This introduces code duplication, which goes against the repository's style guide (L15: 'when you see signs of reuse, modularize it to avoid code duplication').
For better maintainability, it's recommended to avoid repeating these blocks of CSS. While handling both system preference and manual theme selection in pure CSS can be tricky, this duplication can make future updates error-prone and harder to maintain.
Style Guide References
| border: 1px solid var(--color-border-default); | ||
| border-radius: var(--radius-md); | ||
| cursor: pointer; | ||
| transition: all var(--transition-fast); |
There was a problem hiding this comment.
Using transition: all can be inefficient as it causes the browser to watch for changes on all animatable properties, which can negatively impact performance. It's a best practice to specify only the properties you intend to transition. In this case, you are transitioning border-color and box-shadow.1
| transition: all var(--transition-fast); | |
| transition: border-color var(--transition-fast), box-shadow var(--transition-fast); |
Style Guide References
Footnotes
-
Prioritize code readability over code efficiency, but also optimize for efficiency when possible. ↩
1 participant
We've added Default and Compact modes, allowing users to choose how status badges are displayed in the issue list.
Key changes:
fix #33