scale-color $lightness must use $secondary for dark themes #7
Conversation
WalkthroughThis change systematically updates color assignments across numerous SCSS files by replacing direct usage of Changes
Sequence Diagram(s)sequenceDiagram
participant SCSS
participant dark-light-choose
participant ThemeContext
SCSS->>dark-light-choose: Provide (scaled $primary, scaled $secondary)
dark-light-choose->>ThemeContext: Check current theme (light/dark)
ThemeContext-->>dark-light-choose: Return theme mode
dark-light-choose-->>SCSS: Return selected color
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🔭 Outside diff range comments (1)
app/assets/stylesheets/common/base/header.scss (1)
64-66: Dark-theme branch does not switch to$secondary
dark-light-choose(scale-color($header_primary, 50%), $header_primary)keeps$header_primaryfor dark mode instead of moving to a$secondary-based colour as mandated by the PR objective. Was this intentional? It breaks the new convention introduced elsewhere.- color: dark-light-choose(scale-color($header_primary, $lightness: 50%), $header_primary); + color: dark-light-choose(scale-color($header_primary, $lightness: 50%), scale-color($secondary, $lightness: 50%));
♻️ Duplicate comments (1)
app/assets/stylesheets/mobile/login.scss (1)
45-46: Same imbalance of lightness as aboveSee comment on line 17. Re-evaluate whether 50 % vs 50 % would satisfy the new requirement without producing glare in dark mode.
🧹 Nitpick comments (31)
app/assets/stylesheets/common/base/user.scss (2)
122-126: Duplicated colour expression – consider a variableSame compound expression repeated on
.username aand.name. You can reduce noise:$accent-user-name: dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)); .username a, .name { color: $accent-user-name; }
127-131: Minor: keep lightness percentages consistentTitle uses 50 %/50 % whereas the name uses 30 %/70 %. Double-check that difference is intentional and not a copy-paste miss.
app/assets/stylesheets/mobile/user.scss (1)
115-116: Five identical colour literals – extract a helper
dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))is repeated across.instructions, icons, name/title, and user-field copy. Define once:$accent-50: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%));then reuse for clarity & maintainability.
Also applies to: 489-490, 497-498, 503-504, 580-581
app/assets/stylesheets/desktop/user.scss (1)
59-65: Consolidate repeateddark-light-choose(…50%)Same comment as on the mobile sheet – the 50 % flavour appears six times. A single variable or mixin keeps desktop/mobile colour logic aligned:
$accent-50: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%));Also applies to: 173-174, 514-515, 522-523, 528-528, 603-604
app/assets/stylesheets/desktop/upload.scss (1)
20-20: Optional: extract repeated color formula into a variable/mixin
dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%))appears in several places across the PR. Defining a semantic variable or helper (e.g.$text-muted) would avoid repeating the full expression and centralise control if you ever tweak the lightness values.app/assets/stylesheets/mobile/modal.scss (1)
102-102: Consider balancing the two lightness values
scale-color($primary, $lightness: 30%)is considerably darker than$secondaryat 70 %. If the intent is to show a “remaining characters” counter that reads similarly in both themes, you may want closer perceived luminance (e.g. 40 % / 60 %).app/assets/stylesheets/common/base/search.scss (1)
31-38: Repeated colour formulas could be extracted to a variable / mixin
dark-light-choose(scale-color($primary, 40%), scale-color($secondary, 60%))is now duplicated for.blurband.date, while the highlight line uses a very similar expression.
Consider caching the computed colour in a local Sass variable (e.g.$search-blurb-colour) to avoid divergence and simplify future adjustments.app/assets/stylesheets/desktop/topic.scss (1)
60-64: Edge-case: 75 % lightness on$primarymay flatten brand colour
scale-color($primary, $lightness: 75%)pushes the shade extremely close to white, risking loss of brand identity and potential invisibility against light backgrounds.
Suggest capping at ~60 % or switching tomix($primary, $secondary, 70%)for a subtler tint.app/assets/stylesheets/desktop/login.scss (2)
15-19: Minor – consider hover / focus statesAnchor text inside
#login-formnow inherits a dynamic colour but no hover/focus override is provided. Contrast may drop when the link gains the default browser focus outline.
Adding an explicit&:hover, &:focus { text-decoration: underline; }would signal interactivity without having to tweak colours further.
46-49: Consistency with other form labelsMost instructional labels elsewhere (e.g. registration, preferences) still use a static scaled
$primary. Aligning all label colours ondark-light-choose()avoids mismatched hues between dark & light modes.app/assets/stylesheets/desktop/compose.scss (1)
339-341: Color update looks correct – consider contrast testingSwitching to
dark-light-choose()keeps the original 50 % lightness intent while adding dark-theme support. Nothing functionally wrong here, but it would be prudent to run your automated a11y/contrast checks to ensure the icon still meets WCAG AA in both themes.app/assets/stylesheets/common/base/discourse.scss (1)
72-83: Repeated literaldark-light-choose()expressions – extract a Sass variableThe three cold-map rules differ only by the lightness numbers. Consider:
$cold-high: dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%)); $cold-med: dark-light-choose(scale-color($primary, $lightness: 60%), scale-color($secondary, $lightness: 40%)); $cold-low: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); .coldmap-high { color: $cold-high !important; } .coldmap-med { color: $cold-med !important; } .coldmap-low { color: $cold-low !important; }This reduces duplication and makes future palette tweaks easier.
app/assets/stylesheets/common/base/user-badges.scss (1)
95-99: Grant-count colour – minor maintainability noteSame duplication comment as earlier: you might centralise common
dark-light-choose(scale-color(...))values into variables to avoid a search-and-replace hunt next time the palette shifts.app/assets/stylesheets/common/base/topic-post.scss (1)
53-56: Quote title colour – OK, but watch contrastSame caution as before: run contrast checks because quote headers often sit on tinted backgrounds.
app/assets/stylesheets/mobile/login.scss (1)
17-18: Lightness percentages between themes look mismatchedThe light theme variant is only +35 % lighter, while the dark-theme fallback jumps to +65 %. A 30 -point delta risks the two themes diverging in visual hierarchy and can blow past WCAG contrast limits. Unless there is a deliberate design rationale, consider keeping identical percentages (or at most ±5 -10 %) for consistency.
app/assets/stylesheets/mobile/compose.scss (2)
36-37: Very high lightness (+75 %) may wash outA 75 % lightening of
$primarycan produce almost-white text on light themes, harming readability. Consider capping at ~60 % or lowering foreground alpha instead.
51-52: Consistent but duplicated literalThe same 50 %/50 % split is repeated many times across sheets. Extracting a variable (e.g.
$base_link_lightness) would centralise future tweaks.app/assets/stylesheets/common/base/_topic-list.scss (1)
48-52: Header icons split correctly, but leftover primary usage nearby
th button i.fais updated, yet.topic-list.categories .category .badge-notification(line 115) still hard-codesscale-color($primary, 50%). For consistency, consider converting that as well.app/assets/stylesheets/common/base/login.scss (1)
38-40: Consider extracting the repeateddark-light-choose(…50%)expression into a variable or small helper.The exact same two-argument call with 50 % lightness on both
$primaryand$secondaryappears twice just in this small fragment (and dozens of times across the PR). Declaring a local variable, e.g.$accent-50: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%));and then using
color: $accent-50;would:
- Reduce compile-time work for Sass,
- Make the intent obvious,
- Provide a single point of adjustment if design changes.
No functional change – purely a DRY/readability win.
Also applies to: 53-55
app/assets/stylesheets/desktop/topic-list.scss (1)
37-45: High duplication of identicaldark-light-choosecalls – create a helper/mixin.The same pattern
dark-light-choose(scale-color($primary, $lightness: X%), scale-color($secondary, $lightness: X%))is repeated for X = 20, 35, 50, 60, etc. Repetition makes later tweaks error-prone.
Suggestion (one-liner helper):
@function choose-tone($lightness) { @return dark-light-choose( scale-color($primary, $lightness: $lightness), scale-color($secondary, $lightness: $lightness) ); }Then
color: choose-tone(50%);gives the same result with far less noise.Also applies to: 53-56, 130-146, 219-229
app/assets/stylesheets/mobile/topic-post.scss (1)
48-52: Re-use a variable for 50 % & 70 % lightness tones to cut CSS size by ~1 KB.Mobile CSS is size-sensitive. The seven occurrences of the exact 50 % blend and two of the 70 % blend inflate the compiled stylesheet needlessly. Cache them in variables (or via
choose-tone()as proposed earlier) near the top of the file.Also applies to: 147-153, 180-196, 246-249, 268-272, 441-444, 497-500
app/assets/stylesheets/desktop/topic-post.scss (1)
34-35: Desktop topic-post has >40 identical choose calls – factor out to keep the file maintainable.Even a mini-mixin will halve the vertical length of this SCSS and make future palette tweaks (e.g., switching to HSL) a one-line change.
No blocking issues, just a strong maintainability recommendation.
Also applies to: 80-86, 110-116, 124-128, 278-295, 319-334, 340-344, 411-418, 894-899, 940-943, 957-959
app/assets/stylesheets/mobile/topic-list.scss (1)
72-77: Same DRY concern – repeated literal expressions.Follow the helper/variable pattern used in previous comments to avoid drift between desktop & mobile colour definitions.
Also applies to: 94-97, 144-148, 206-211
app/assets/stylesheets/desktop/modal.scss (3)
64-72: Prefer a dedicated variable for repeated colourThe same
dark-light-choose(scale-color($primary, 35%), scale-color($secondary, 65%))appears in other files. Define a semantic variable (e.g.$modal-link) once and reuse – keeps themes DRY and future-proof.
113-122: Hover/normal colours should preserve perceptual orderThe hover state (line 121) becomes darker than the normal (line 117). For light themes this is fine, but in dark themes the perceived order flips (lighter on hover). Swap the percentages (60 % vs 40 %) or use
darken()/lighten()for clearer intent.
127-132: Minor: alpha-reuse instead of 60 %/40 % pairIf the intent is a single colour with different opacity on light/dark themes, consider
rgba()withdark-light-diffinstead of two separate scaled colours – improves maintainability.app/assets/stylesheets/common/admin/admin_base.scss (5)
384-386: Use a shared “subdued-text” token
dark-light-choose(scale-color($primary, 50%), scale-color($secondary, 50%))is now repeated at least four times in this file; define$subdued-textin variables to keep admin theme tweaks centralized.
522-525: Border colour may become invisible in dark schemesA top border at 80 %/20 % lightness against
$secondarycan practically vanish when$secondaryis close to the chosen dark variant. Considerdark-light-diff()instead ofdark-light-choose()for borders.
600-601: Single source of truth for input bordersSame comment as lines 576-578: extract to
$input-bordervariable; avoids divergent tweaks between.groupssections.
1293-1295: Hover/background mismatchFilter tags use
dark-light-choosebut hover state (line 1301) switches todark-light-diff, creating a perceptual jump between normal/hover in dark mode. Harmonise the two helpers.
1330-1332: Dot-border on large lists can be heavyA dotted
1pxborder on every list container impacts rendering performance when thousands of rows are virtual-scrolled. Consider a lighter solid colour or remove entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
app/assets/stylesheets/common/admin/admin_base.scss(8 hunks)app/assets/stylesheets/common/base/_topic-list.scss(4 hunks)app/assets/stylesheets/common/base/discourse.scss(1 hunks)app/assets/stylesheets/common/base/header.scss(4 hunks)app/assets/stylesheets/common/base/login.scss(2 hunks)app/assets/stylesheets/common/base/notification-options.scss(1 hunks)app/assets/stylesheets/common/base/search.scss(1 hunks)app/assets/stylesheets/common/base/share_link.scss(1 hunks)app/assets/stylesheets/common/base/topic-post.scss(4 hunks)app/assets/stylesheets/common/base/user-badges.scss(2 hunks)app/assets/stylesheets/common/base/user.scss(1 hunks)app/assets/stylesheets/common/components/badges.css.scss(2 hunks)app/assets/stylesheets/common/components/buttons.css.scss(1 hunks)app/assets/stylesheets/desktop/compose.scss(1 hunks)app/assets/stylesheets/desktop/header.scss(1 hunks)app/assets/stylesheets/desktop/login.scss(2 hunks)app/assets/stylesheets/desktop/modal.scss(5 hunks)app/assets/stylesheets/desktop/queued-posts.scss(1 hunks)app/assets/stylesheets/desktop/topic-list.scss(5 hunks)app/assets/stylesheets/desktop/topic-post.scss(13 hunks)app/assets/stylesheets/desktop/topic.scss(1 hunks)app/assets/stylesheets/desktop/upload.scss(1 hunks)app/assets/stylesheets/desktop/user.scss(4 hunks)app/assets/stylesheets/embed.css.scss(1 hunks)app/assets/stylesheets/mobile/compose.scss(3 hunks)app/assets/stylesheets/mobile/login.scss(2 hunks)app/assets/stylesheets/mobile/modal.scss(1 hunks)app/assets/stylesheets/mobile/topic-list.scss(4 hunks)app/assets/stylesheets/mobile/topic-post.scss(7 hunks)app/assets/stylesheets/mobile/topic.scss(1 hunks)app/assets/stylesheets/mobile/upload.scss(1 hunks)app/assets/stylesheets/mobile/user.scss(3 hunks)
🔇 Additional comments (31)
app/assets/stylesheets/mobile/upload.scss (1)
10-10: Dynamic theming hook looks correct
dark-light-choose()cleanly swaps the hard-codedscale-color($primary…)for a light/dark aware choice with matching lightness. Syntax & parameters look valid – nothing else to do here.app/assets/stylesheets/common/base/user.scss (1)
117-121: Confirm contrast for light mode
dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%))may yield two very different luminosities (–30 % vs +70 %). In light mode the primary branch (-30 %) will be quite dark, which could harm readability on dark backgrounds.
Please eyeball the contrast or run the SCSS against the default palette before merging.app/assets/stylesheets/desktop/queued-posts.scss (1)
14-15: LGTM – theme-aware colour addedThe switch to
dark-light-choose()is syntactically correct and keeps lightness parity.app/assets/stylesheets/common/base/notification-options.scss (1)
2-2: LGTM – dynamic colour looks correctThe new
dark-light-choosecall matches the signature used elsewhere and preserves the intended muted tone.app/assets/stylesheets/mobile/topic.scss (1)
34-34: Verify contrast for accessibilityPrimary + 75 % lightness vs secondary + 25 % lightness could yield very low contrast in one of the two themes, depending on your base palette. It might drop below WCAG AA for small text. Please run a quick contrast check on
$primary/$secondarywith these adjustments.app/assets/stylesheets/common/base/share_link.scss (1)
62-62: LGTM – consistent with the new theming ruleNo issues spotted; the conditional colour keeps the subtle date styling while supporting dark mode.
app/assets/stylesheets/desktop/header.scss (1)
52-60: Confirmdark-light-choose()generates WCAG-compliant contrast for both themesThe new foreground shades (
45 %lightened primary vs.55 %lightened secondary, and25 %vs.75 %in the highlight) are substantially lighter/darker than before.
Please verify – especially against the existing background for.blurb– that contrast ratios remain ≥ 4.5:1 in both light & dark modes; otherwise users with low vision may struggle to read blurbs and highlighted terms.app/assets/stylesheets/desktop/topic.scss (1)
67-70: Keep theme symmetryFor
a.reply-newboth branches use50 %lightness, but the “light” branch uses$primarywhile the “dark” branch also lightens$secondary.
Confirm that$secondaryat 50 % doesn’t collide with$tertiaryhover state (line 80) or$secondarybackground (line 72) when themes swap.app/assets/stylesheets/common/components/buttons.css.scss (1)
60-61: Check disabled-button text contrast in dark modeWhen disabled, the button background comes from
dark-light-diff($primary, $secondary, 90%, -60%).
The new hover text colour for disabled buttons choosesscale-color($secondary, 30%)in dark mode, which may be too dark against that background, making the label unreadable. Please run a quick contrast check or adjust the percentage.app/assets/stylesheets/embed.css.scss (1)
88-90: 👍 Theme-aware colour for new usersThe new
dark-light-choose()call keeps the 70 %/30 % balance and adds dark-theme parity. No issues spotted.app/assets/stylesheets/common/base/user-badges.scss (1)
59-63: Badge count colour updated correctlyImplementation aligns with the new theming convention; no behavioural change beyond colour selection.
app/assets/stylesheets/common/base/topic-post.scss (5)
14-16: Link colour update is soundAdds dark-theme support while preserving the original visual weight.
20-22: Icon colour update follows conventionConsistent with neighbouring changes.
23-25:new_user/ title colour – OKNo functional concerns.
70-72: Quote-controls colour – goodMatches overall theming strategy.
158-160:via-emailinfo colour – OKNo concerns.
app/assets/stylesheets/common/base/header.scss (3)
151-156: Topic-count colour updated – looks goodUniform percentages and correct switch to
$secondaryfor dark themes. No issues spotted.
301-303: Great use of helper for inline iconsConsistent 50 %/50 % split keeps list-item states coherent across themes.
195-197: Dark-light-choose helper is defined and correctly importedThe
dark-light-choosefunction lives exactly once in
•app/assets/stylesheets/common/foundation/variables.scssIt’s pulled in via
•@import "common/foundation/base"(which importsvariables.scssthenmixins.scss)
• that base manifest is loaded first inapp/assets/stylesheets/common.scss, and thatcommon.scssis imported by yourmobile.scss,desktop.scss, andadmin.scssmanifests.Additionally,
embed.css.scssexplicitly does@import "./common/foundation/variables"; @import "./common/foundation/mixins";so it too has the helper available.
All your new calls to
dark-light-choose(...)incommon/base/header.scss(and across other partials) will resolve without Sass compilation errors.app/assets/stylesheets/mobile/compose.scss (1)
168-173: Background for disabled inputs – good catchSwitching both branches keeps disabled fields legible in both themes. No issues.
app/assets/stylesheets/common/components/badges.css.scss (2)
239-240: Background may clash with constant$secondarytext
badge-notificationkeeps$secondarytext colour even when the dark branch darkens the background toward$secondary(30 %). Check contrast ratios; you may need to invert text to$primaryfor that branch.
285-286: 👍 Posts badge now theme-awareThe switch to
dark-light-choosefor link colour aligns with the new guideline. Looks correct.app/assets/stylesheets/common/base/_topic-list.scss (4)
55-57: Table body colour updated – LGTMNo further concerns; consistent with new pattern.
67-70: Excerpt colour matches headersGood thematic consistency maintained.
245-247: Breadcrumb date uses 60 %/40 % split – compatibleBalanced percentages, no issues observed.
300-302: Education notice converted – conformsUpdate is straight-forward and correct.
app/assets/stylesheets/desktop/modal.scss (2)
92-96: Accessibility: re-evaluate 70 %/30 % contrast in dark modeA 70 % lightness shift on
$primarymay still be too low-contrast on the$secondarybackground for WCAG AA (especially for hint text at 85 % font-size). Consider bumping the delta or running a contrast checker.
49-53: dark-light-choose helper is defined and correctly imported
Thedark-light-choose($light, $dark)function lives inapp/assets/stylesheets/common/foundation/variables.scss(lines 46–48) with matching positional parameters. Your global Sass manifest already loads the foundation variables beforemodal.scss, so there will be no build‐time error.app/assets/stylesheets/common/admin/admin_base.scss (3)
504-507: WCAG contrast for help textHelp text is small (0.9 em). 40 %/60 % scaling can fall below 4.5:1 on many palettes. Please run a checker or bump the lightness delta.
576-578: Keep visual focus-ring width consistentChanging only border colour here without adjusting width/box-shadow may impact focus visibility for keyboard users. Review in dark mode.
726-727: Column descriptions – low priorityNo functional issues; change is consistent with earlier subdued-text colour.
2 participants
Test 7
Summary by CodeRabbit