-
Notifications
You must be signed in to change notification settings - Fork 0
scale-color $lightness must use $secondary for dark themes #4
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?
scale-color $lightness must use $secondary for dark themes #4
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant SCSS as SCSS Files
participant Func as dark-light-choose()
participant Theme as Theme Engine
participant Browser as Browser
Dev->>SCSS: Update scale-color() calls
SCSS->>SCSS: Wrap with dark-light-choose()
Note over SCSS: Original: scale-color($primary, $lightness: X%)
Note over SCSS: New: dark-light-choose(scale-color($primary, $lightness: X%), scale-color($secondary, $lightness: (100-X)%))
SCSS->>Func: Pass light & dark theme values
Func->>Func: Check brightness($primary) vs brightness($secondary)
alt Light Theme (primary darker)
Func->>Theme: Return first param (primary-based)
else Dark Theme (primary lighter)
Func->>Theme: Return second param (secondary-based)
end
Theme->>Browser: Apply computed color values
Browser->>Browser: Render with theme-appropriate colors
|
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.
32 files reviewed, 1 comment
| a { | ||
| font-weight: bold; | ||
| color: scale-color($primary, $lightness: 30%); | ||
| color: dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%)); |
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: Incorrect lightness inversion - original was $lightness: 30% on $primary, but changed to $lightness: 70% on $primary for light theme. Should preserve the original lightness value.
| color: dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%)); | |
| color: dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/stylesheets/desktop/topic-post.scss
Line: 291:291
Comment:
**logic:** Incorrect lightness inversion - original was `$lightness: 30%` on `$primary`, but changed to `$lightness: 70%` on `$primary` for light theme. Should preserve the original lightness value.
```suggestion
color: dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%));
```
How can I resolve this? If you propose a fix, please make it concise.
Test 7
Replicated from ai-code-review-evaluation/discourse-greptile#7