-
Notifications
You must be signed in to change notification settings - Fork 9
scale-color $lightness must use $secondary for dark themes #7
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: theme-color-scheme-pre
Are you sure you want to change the base?
Conversation
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.
Greptile Summary
This PR implements a comprehensive theme color scheme refactoring across 32 SCSS files to fix dark theme compatibility issues. The changes systematically replace hardcoded scale-color($primary, $lightness: X%) calls with dark-light-choose() function calls that provide theme-aware color selection.
The core issue being addressed is that $primary and $secondary colors have different semantic meanings in light vs dark themes. In light themes, $primary is typically dark text on light backgrounds, while in dark themes, $primary becomes light text on dark backgrounds. When using scale-color($primary, $lightness: X%) universally, dark themes would scale already-light colors to become even lighter, resulting in poor contrast and readability.
The dark-light-choose() function detects theme type by comparing the brightness of $primary and $secondary colors. For light themes, it uses the first parameter (scaled $primary), and for dark themes, it uses the second parameter (scaled $secondary). The lightness percentages are often adjusted between themes to maintain proper contrast ratios - for example, 70% lightness in light themes becomes 30% lightness in dark themes.
This change affects UI elements across the entire application including:
- Text colors for usernames, dates, links, and secondary content
- Background colors for badges, notifications, and form elements
- Icon colors in headers, topic lists, and user interfaces
- Border and highlight colors in search results and modals
The refactoring follows consistent patterns throughout the codebase and aligns with Discourse's existing theme system architecture. This systematic approach ensures all UI components maintain appropriate visual hierarchy and accessibility standards across both light and dark color schemes.
Confidence score: 4/5
• This PR is generally safe to merge as it follows established patterns and addresses a legitimate theming issue
• The confidence score reflects the extensive scope of changes across 32 files, which increases the potential for visual inconsistencies if not thoroughly tested
• Files requiring extra attention: app/assets/stylesheets/common/base/topic-post.scss and app/assets/stylesheets/mobile/topic-post.scss due to inconsistencies in lightness value changes that may affect visual hierarchy
31 files reviewed, 2 comments
| } | ||
| .new_user a, .user-title, .user-title a { | ||
| color: scale-color($primary, $lightness: 50%); | ||
| color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); |
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.
style: Both light and dark theme variants use 50% lightness - consider if this provides sufficient contrast differentiation between themes
| margin-bottom: 4px; | ||
| margin-top: 0; | ||
| color: scale-color($primary, $lightness: 20%); | ||
| color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); |
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.
logic: Lightness value changed from 20% to 50% - this may affect visual hierarchy as h3 headings will now have the same color intensity as buttons and other UI elements
Test 7