scale-color $lightness must use $secondary for dark themes#7
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR systematically replaces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 (4)
app/assets/stylesheets/common/base/_topic-list.scss (1)
48-55: Consider extracting repeated color expression into a variable.The expression
dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))is repeated 5+ times in this file (lines 48, 51, 55, 69, 301). Consider defining a SCSS variable at the top of the file to improve maintainability.♻️ Suggested refactor
+$muted-text-color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); + .topic-list { // ... th { - color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); + color: $muted-text-color; font-weight: normal; font-size: 1em; - button i.fa {color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%));} + button i.fa {color: $muted-text-color;} } td { - color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); + color: $muted-text-color; font-size: 1em; }app/assets/stylesheets/mobile/user.scss (1)
572-586: Consider extracting the repeated color expression.This file uses the same
dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))expression 5 times. A shared variable would reduce duplication.app/assets/stylesheets/desktop/topic-list.scss (1)
37-55: Consider defining a shared variable for the repeated color expression.The expression
dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))appears 8 times in this file. A variable would improve maintainability.app/assets/stylesheets/mobile/compose.scss (1)
33-33: Consider updating#file-uploadingfor consistency.Line 33 uses
scale-color($secondary, $lightness: 50%)directly, while the adjacent#draft-statuson line 36 now usesdark-light-choose. For visual consistency in dark themes, this selector may benefit from the same treatment.🔧 Suggested change
#file-uploading { left: 51%; font-size: 0.857em; - color: scale-color($secondary, $lightness: 50%); + color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
app/assets/stylesheets/common/admin/admin_base.scssapp/assets/stylesheets/common/base/_topic-list.scssapp/assets/stylesheets/common/base/discourse.scssapp/assets/stylesheets/common/base/header.scssapp/assets/stylesheets/common/base/login.scssapp/assets/stylesheets/common/base/notification-options.scssapp/assets/stylesheets/common/base/search.scssapp/assets/stylesheets/common/base/share_link.scssapp/assets/stylesheets/common/base/topic-post.scssapp/assets/stylesheets/common/base/user-badges.scssapp/assets/stylesheets/common/base/user.scssapp/assets/stylesheets/common/components/badges.css.scssapp/assets/stylesheets/common/components/buttons.css.scssapp/assets/stylesheets/desktop/compose.scssapp/assets/stylesheets/desktop/header.scssapp/assets/stylesheets/desktop/login.scssapp/assets/stylesheets/desktop/modal.scssapp/assets/stylesheets/desktop/queued-posts.scssapp/assets/stylesheets/desktop/topic-list.scssapp/assets/stylesheets/desktop/topic-post.scssapp/assets/stylesheets/desktop/topic.scssapp/assets/stylesheets/desktop/upload.scssapp/assets/stylesheets/desktop/user.scssapp/assets/stylesheets/embed.css.scssapp/assets/stylesheets/mobile/compose.scssapp/assets/stylesheets/mobile/login.scssapp/assets/stylesheets/mobile/modal.scssapp/assets/stylesheets/mobile/topic-list.scssapp/assets/stylesheets/mobile/topic-post.scssapp/assets/stylesheets/mobile/topic.scssapp/assets/stylesheets/mobile/upload.scssapp/assets/stylesheets/mobile/user.scss
🔇 Additional comments (90)
app/assets/stylesheets/mobile/upload.scss (1)
9-11: LGTM!The
dark-light-choosehelper correctly applies theme-aware color scaling: using$primaryfor light themes and$secondaryfor dark themes. This ensures the.descriptiontext maintains appropriate contrast in both theme contexts, consistent with the PR-wide pattern.app/assets/stylesheets/mobile/modal.scss (1)
100-104: The change is correct. Thedark-light-choosefunction signature is($light-theme-result, $dark-theme-result), and the code correctly placesscale-color($secondary, $lightness: 70%)in the dark-theme slot (second argument), which aligns with the PR requirement to use$secondaryfor dark themes.app/assets/stylesheets/desktop/compose.scss (1)
339-341: LGTM!The
dark-light-choosewrapper correctly provides theme-aware color selection for the reply-to icon, using consistent 50% lightness for both primary and secondary color branches.app/assets/stylesheets/common/base/notification-options.scss (1)
1-3: LGTM!The inverted lightness values (40% for primary in light mode, 60% for secondary in dark mode) appropriately ensure the muted icon maintains consistent visual subtlety across both theme contexts.
app/assets/stylesheets/mobile/topic.scss (1)
34-34: LGTM!The
dark-light-choosewrapper with inverted lightness values (75%/25%) correctly adapts the private message glyph color for both themes. This is consistent with the desktop equivalent indesktop/topic.scss.app/assets/stylesheets/desktop/queued-posts.scss (1)
14-14: LGTM!The theme-aware color update for post info spans correctly applies the
dark-light-choosepattern with consistent 50% lightness scaling.app/assets/stylesheets/desktop/topic.scss (2)
60-64: LGTM!The private message glyph color correctly uses inverted lightness values (75%/25%) for proper contrast in both themes, matching the mobile equivalent.
67-78: LGTM!The
a.reply-newselector correctly applies thedark-light-choosepattern with consistent 50% lightness values, maintaining a uniform appearance across themes.app/assets/stylesheets/embed.css.scss (1)
88-90: LGTM!The
dark-light-choosewrapper correctly provides theme-aware color contrast. The complementary lightness values (70% for primary in light themes, 30% for secondary in dark themes) ensure the "new-user" link appears appropriately muted in both contexts.app/assets/stylesheets/desktop/upload.scss (1)
19-21: LGTM!Using symmetric 50% lightness for both primary and secondary ensures consistent mid-tone contrast for description and hint text across light and dark themes.
app/assets/stylesheets/common/components/buttons.css.scss (1)
58-62: LGTM!The
dark-light-choosewrapper for the disabled button hover state correctly maintains muted text contrast in both light and dark themes using the complementary 70%/30% lightness pattern.app/assets/stylesheets/common/components/badges.css.scss (2)
239-239: LGTM!The
dark-light-choosewrapper with complementary 70%/30% lightness values provides appropriate muted background color for notification badges across both theme contexts.
284-286: LGTM!The 40%/60% complementary lightness pattern ensures the badge-posts link color maintains readable contrast—darker primary in light themes, lighter secondary in dark themes.
app/assets/stylesheets/common/base/share_link.scss (1)
59-63: LGTM!Symmetric 50% lightness for both primary and secondary provides consistent mid-tone styling for date metadata in both light and dark themes.
app/assets/stylesheets/desktop/header.scss (1)
52-61: LGTM!The
dark-light-choosewrapper correctly uses complementary lightness values (45%/55% and 25%/75%) to ensure proper contrast for both light and dark themes. The pattern is consistent with the broader refactor across the codebase.app/assets/stylesheets/common/base/user-badges.scss (2)
59-63: LGTM!Using equal 50% lightness for both primary and secondary creates a consistent mid-tone gray regardless of theme, which is appropriate for a subtle count indicator.
95-98: LGTM!The complementary lightness values (40%/60%) ensure proper contrast in both themes.
app/assets/stylesheets/desktop/modal.scss (5)
49-53: LGTM!The complementary lightness values (70%/30%) provide appropriate subdued hint text styling for both themes.
63-72: LGTM!Close button uses complementary lightness (35%/65%) with a proper hover state that transitions to
$primary.
92-96: LGTM!Consistent with the hint styling pattern (70%/30%).
112-125: LGTM!The hover state correctly uses darker/lighter values (40%/60%) compared to the default state (50%/50%), providing appropriate visual feedback.
127-132: LGTM!Complementary lightness values (60%/40%) are appropriate for subdued reason text.
app/assets/stylesheets/common/base/search.scss (1)
26-39: LGTM!The color hierarchy is well-structured:
.blurband.dateshare the same subdued styling (40%/60%).search-highlightuses higher contrast (25%/75%) to emphasize matched textapp/assets/stylesheets/desktop/topic-post.scss (10)
29-35: LGTM!The reply-to-tab uses neutral 50%/50% lightness for consistent mid-tone styling.
80-87: LGTM!Post controls use appropriate contrast levels - highlight-action at 60%/40% and general links/buttons at 75%/25%.
110-128: LGTM!The create button correctly uses high contrast (20%/80%) to stand out as a primary action.
276-295: LGTM!Embedded posts styling uses consistent complementary lightness values for dates, links, and arrows.
319-343: LGTM!Topic map headings use appropriate hierarchy:
h3at 20%/80% for primary headingsh4at 50%/50% for secondary headingsspan.domainat 50%/50% for subdued metadata
378-384: LGTM!Numbers and icons use high contrast (20%/80%) for visibility.
411-419: LGTM!Button color uses 60%/40% with proper hover state transition to
$primary.
893-900: LGTM!Select-posts buttons use consistent styling with complementary lightness values for background and border.
936-942: LGTM!Post info links use neutral 50%/50% for consistent subdued appearance.
957-959: LGTM!New user styling uses 70%/30% for appropriately subdued display.
app/assets/stylesheets/common/base/_topic-list.scss (4)
111-116: Potential inconsistency with unchanged code.Line 115 uses
scale-color($primary, $lightness: 50%)without thedark-light-choosewrapper, while similar usages elsewhere in this file have been updated. This may result in inconsistent theming behavior for.badge-notificationwithin.topic-list.categories.
245-250: LGTM!The asymmetric lightness values (60%/40%) for
.top-date-stringappropriately adjust contrast for both light and dark themes.
66-73: LGTM!The
.topic-excerptcolor update follows the established pattern for dark/light theme support.
300-302: LGTM!The
div.educationcolor update is consistent with the theming changes throughout the file.app/assets/stylesheets/mobile/user.scss (3)
114-120: LGTM!The
.instructionscolor update correctly applies the dark-light-choose pattern.
487-492: LGTM!The notification icon color update follows the PR pattern. The inline comment referencing
common/base/header.scssis helpful for cross-file consistency tracking.
493-505: LGTM!The
.group-member-info.nameand.titlecolor updates are consistent with the theming changes.app/assets/stylesheets/desktop/topic-list.scss (4)
37-42: LGTM!The icon color updates for
.fa-thumb-tack,.fa-thumb-tack.unpinned, and.fa-bookmarkcorrectly implement the dark-light-choose pattern.
53-55: LGTM!The table header button icon color update is consistent with other theming changes.
132-148: LGTM!The
.post-actionscolor updates for both the container and anchor elements follow the established pattern.
219-234: Verify necessity of!important.Line 223 uses
!importanton the icon color. If this was pre-existing, it's fine; otherwise, consider whether specificity issues can be resolved without!important.app/assets/stylesheets/common/base/user.scss (1)
117-130: LGTM!The differentiated lightness values are appropriate:
.username aand.nameuse 30%/70% for stronger contrast on important identifiers.titleuses 50%/50% for a more muted appearance on secondary informationapp/assets/stylesheets/common/base/login.scss (2)
38-42: LGTM!The
.controls pcolor update follows the standard muted text pattern.
47-55: LGTM!The 35%/65% lightness values appropriately de-emphasize the alternate login/account buttons, aligning with the comment on line 47.
app/assets/stylesheets/mobile/login.scss (2)
17-17: LGTM!The
dark-light-choosewrapper correctly applies complementary lightness values (35%/65%) for proper contrast in both light and dark themes.
45-45: LGTM!Consistent use of
dark-light-choosewith balanced 50%/50% lightness for instruction labels.app/assets/stylesheets/common/base/topic-post.scss (6)
15-15: LGTM!Correct application of
dark-light-choosewith 30%/70% complementary lightness for name link colors.
21-21: LGTM!Appropriate 40%/60% lightness split for FontAwesome icon colors.
23-25: LGTM!Balanced 50%/50% lightness for user-title elements provides consistent muted appearance across themes.
55-55: LGTM!Quote title color follows the same 30%/70% pattern as other prominent text elements.
71-71: LGTM!The inverted 70%/30% pattern here intentionally makes quote-controls more subtle/muted, which is appropriate for secondary UI elements.
159-159: LGTM!Consistent with quote-controls, the via-email indicator uses the 70%/30% inverted pattern for a subtler appearance.
app/assets/stylesheets/mobile/topic-list.scss (4)
75-76: LGTM!Standard 50%/50% lightness for table cell text provides consistent muted appearance across themes.
94-96: LGTM!Icon colors in topic stats correctly use the balanced 50%/50% pattern.
145-147: LGTM!Featured topic timestamp links use consistent 50%/50% lightness.
206-212: LGTM!The 10%/90% extreme lightness split combined with
font-weight: boldcreates high-contrast emphasis text for category figures, which is appropriate for this prominent UI element.app/assets/stylesheets/desktop/login.scss (2)
17-17: LGTM!Matches the mobile login stylesheet pattern with 35%/65% complementary lightness for login form links.
48-48: LGTM!Instruction label colors use balanced 50%/50% lightness, consistent with the mobile counterpart.
app/assets/stylesheets/mobile/topic-post.scss (8)
49-49: LGTM!Base button color uses standard 50%/50% balanced lightness for consistent appearance across themes.
151-151: LGTM!Reply-to-tab link color follows the standard 50%/50% pattern.
182-183: LGTM!Topic map heading colors use balanced 50%/50% lightness.
190-190: LGTM!Consistent with h3, the h4 uses the same 50%/50% pattern.
246-249: LGTM!The 20%/80% split for map numbers and icons provides stronger contrast than standard muted text, appropriate for these interactive/informational elements.
270-270: LGTM!The inverted 75%/25% pattern for
.domaincreates intentionally muted/subtle text for domain display.
442-442: LGTM!Topic admin button uses standard 50%/50% balanced lightness.
498-498: LGTM!The inverted 70%/30% pattern for new-user links intentionally creates a subtler appearance, visually distinguishing new users.
app/assets/stylesheets/desktop/user.scss (5)
59-59: LGTM!The
dark-light-choosehelper is correctly applied to provide theme-aware text color for.instructions.
173-173: LGTM!Consistent application of
dark-light-choosefor table header text color.
514-514: LGTM!Icon color now adapts correctly to light/dark themes.
522-528: LGTM!Both
.nameand.titlewithin.group-member-infoconsistently usedark-light-choosewith symmetric lightness values.
603-603: LGTM!The
.controls ptext color is correctly updated to support dark/light theme switching.app/assets/stylesheets/common/admin/admin_base.scss (8)
385-386: LGTM!The
.desctext color correctly usesdark-light-choosewith symmetric 50%/50% lightness for balanced contrast in both themes.
504-508: LGTM!The asymmetric lightness values (40%/60%) for
p.helpare appropriate—lighter primary in light mode, darker secondary in dark mode provides good readability.
524-525: LGTM!Border color uses 80%/20% lightness values to ensure the separator is visible in both light and dark contexts.
577-577: LGTM!The
.ac-wrapborder color uses 75%/25% lightness for theme-appropriate border visibility.
600-600: LGTM!Consistent with the
.ac-wrappattern for.select2-choicesborder.
726-726: LGTM!The
.descriptioncolor uses symmetric 50%/50% lightness for balanced text contrast.
1294-1294: LGTM!The filter link background color uses 75%/25% lightness to provide appropriate contrast for the background in both themes.
1331-1331: LGTM!Consistent border styling with 75%/25% lightness values.
app/assets/stylesheets/common/base/discourse.scss (1)
75-83: LGTM!The coldmap classes correctly use
dark-light-choosewith graduated asymmetric lightness values (70%/30%, 60%/40%, 50%/50%) to maintain the "dimming" visual hierarchy for cold topics in both light and dark themes.app/assets/stylesheets/common/base/header.scss (4)
152-156: LGTM!The
.topics-countbadge text color correctly adapts to light/dark themes with symmetric lightness.
195-196: LGTM!The notifications dropdown uses appropriate lightness values—symmetric 50%/50% for
.faicons and asymmetric 30%/70% for.iconto ensure proper visibility of actionable icons in both themes.
300-305: LGTM!The
.badge-notificationtext color within.categorycorrectly uses thedark-light-choosehelper.
329-337: LGTM!The
.topic-statusescolor uses symmetric 50%/50% lightness for consistent appearance across themes.app/assets/stylesheets/mobile/compose.scss (3)
36-37: LGTM!The
#draft-statustext color uses 75%/25% lightness for subtle, theme-aware text in both modes.
51-52: LGTM!The
.togglercolor uses symmetric 50%/50% lightness for balanced visibility.
168-173: LGTM!Both
#reply-title:disabledand.wmd-input:disabledconsistently usedark-light-choosewith 75%/25% lightness for disabled state background colors, ensuring proper visual feedback in both themes.
2 participants
Test 7
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#7