Discourse / PR #31

scale-color $lightness must use $secondary for dark themes

discourse/discourse@d38c4d5f
Golden recall: 67% Extra findings: 9

Golden Comments (2/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 In .topic-meta-data h5 a, the original code had color: scale-color($primary, $lightness: 30%) but was changed to dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%)). The lightness for the light theme changed from 30% to 70%, which is a dramatic inversion LOW ✓ Found "The embedded-post author link's light-theme lightness was unintentionally changed from 30% to 70% while adding the dark arm (dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%))). This is a light-theme visual regression and makes the dark value inconsistent with other embedded-posts links." (app/assets/stylesheets/desktop/topic-post.scss L291)
#2 This change for desktop/user.css changes $primary from 30% to 50% for the light theme; most other changes preserve the original $primary value and move the complement to $secondary for dark. Consider reviewing this (also applies to a similar .name change in the mobile variant). LOW ✓ Found ".group-member-info .name light-theme lightness was silently changed from 30% to 50% during conversion, causing a light-theme regression and inconsistency with common/base/user.scss (which uses 30%/70%)." (app/assets/stylesheets/desktop/user.scss L522) — and the mobile variant: "Same regression as desktop: .group-member-info .name light-theme lightness was changed from 30% to 50%, diverging from common/base/user.scss (30%/70%)." (app/assets/stylesheets/mobile/user.scss L497). The golden's "also applies to the mobile variant" is covered by the second candidate.
#3 In topic-post.css the original code used $lightness: 70% but the replacement uses $lightness: 30% for the light theme. This makes the text significantly darker than intended. LOW ✗ Not found No candidate matches a 70→30 inversion inside topic-post: the two regressions the review flags in that file both run in the opposite direction (30→70 at desktop/topic-post.scss L291, 20→50 at mobile/topic-post.scss L182), and the only 70→30 inversion it reports is .custom-message-length in mobile/modal.scss L102, outside topic-post. See app/assets/stylesheets/desktop/topic-post.scss and app/assets/stylesheets/mobile/topic-post.scss

Supplementary Findings (9 findings)

Finding File Severity Legitimate?
.custom-message-length uses dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)), inverted relative to the desktop counterpart (desktop/modal.scss line 94, 70%/30%) — the light theme gets the darker 30% value instead of the original 70% app/assets/stylesheets/mobile/modal.scss:102 HIGH ✓ Real: light-theme regression plus a desktop/mobile consistency break
The topic-map h3 light-theme lightness was changed from 20% to 50% while adding the dark arm, inconsistent with desktop/topic-post.scss (line 321, which correctly keeps 20%/80%) app/assets/stylesheets/mobile/topic-post.scss:182 HIGH ✓ Real: light-theme regression and divergence from the desktop counterpart
.edit-category-modal .badge-list li a { color: #888; } was not converted to dark-light-choose, unlike its desktop counterpart (desktop/modal.scss line 116) — the hardcoded #888 does not adapt to dark themes app/assets/stylesheets/mobile/modal.scss:126 MEDIUM ✓ Real: hardcoded color diverges from the converted desktop rule and stays light-on-dark
The mobile composer status text (#draft-status line 36, .toggler line 51) derives from $secondary on dark themes while desktop leaves it at $primary — ~25% text on a ~40% dark surface risks low/inverted contrast app/assets/stylesheets/mobile/compose.scss:36 MEDIUM ✓ Borderline: the desktop/mobile inconsistency is real; the contrast risk is estimated, not measured
The category-pills .badge-notification still uses color: scale-color($primary, $lightness: 50%) without a dark-theme $secondary arm — renders as a light-gray badge-on-dark on dark themes app/assets/stylesheets/common/base/_topic-list.scss:115 MEDIUM ✓ Real: missing dark-theme arm, inconsistent with the rest of the topic list
Disabled composer inputs use a different dark mechanism than desktop: mobile #reply-title:disabled/.wmd-input:disabled use dark-light-choose(scale-color($primary, 75%), scale-color($secondary, 25%)) while desktop uses dark-light-diff($primary, $secondary, 90%, -60%) — both dark-aware, but tint strength differs across breakpoints app/assets/stylesheets/mobile/compose.scss:168 LOW ✓ Borderline: the review itself says "not a bug" — a consistency preference
Several converted one-liners (lines 113, 294, 941) have a stray space before the closing brace (e.g. ...%)); }), inconsistent with the file convention (...%));}) app/assets/stylesheets/desktop/topic-post.scss:113 LOW ✓ Real but cosmetic: trailing whitespace only
The .badge-notification background was made theme-aware, but its color: $secondary (line 236) remains static, yielding low contrast in light (~2:1 on the light-gray chip) and dark (~3:1) themes app/assets/stylesheets/common/components/badges.css.scss:239 LOW ✓ Borderline: pre-existing contrast note, explicitly polish-only
The rewritten rule span {color: dark-light-choose(...); } has a trailing space before the closing brace, inconsistent with surrounding formatting; the 50%/50% functional change is correct app/assets/stylesheets/desktop/queued-posts.scss:14 LOW ✓ Real but cosmetic: trailing whitespace only

Honest Analysis

Two of the three goldens are found, and the second one is found better than written: G1 lands on desktop/topic-post.scss L291 with the exact same values as the golden (light theme 30%→70%, dark-light-choose 70/30); G2 is covered by two explicit candidates, desktop/user.scss L522 and mobile/user.scss L497 — the golden only mentions the mobile variant in passing, the review names both. G3 is a genuine miss: nothing in the review reports a 70→30 light-theme darkening inside topic-post; the two regressions it flags in that file run in the opposite direction (30→70 at desktop L291, 20→50 at mobile L182), and its only 70→30 inversion is in mobile/modal.scss L102, outside the golden's file.

The nine supplementary findings are a mixed bag. The two HIGH items are real: the modal.scss L102 inversion (desktop/mobile inconsistency plus a light-theme regression) and the topic-map 20→50 change diverging from desktop's 20/80. The incomplete conversions (#888 at mobile/modal.scss L126, the badge-notification at _topic-list.scss L115) are genuine gaps against the PR's own stated goal. But five of the nine are formatting nits or explicitly hedged items — trailing spaces, brace spacing, "polish only" contrast notes — which inflate the count on a colors-only PR.

Two honest caveats. First, the review's own summary is contradictory: it opens by asserting the pattern is "applied consistently and correctly across the majority of the reviewed stylesheets" and that the changes "preserve the existing light-theme rendering", then lists five findings that directly contradict that — every light-theme regression it flags is a case of not preserving the rendering. The summary overstates the clean state the body then dismantles. Second, the G3 miss shows the sweep was not exhaustive: the review examined the files it flagged in depth but did not enumerate every scale-color call site in topic-post, letting the one inversion in the golden's direction slip through. Overall: strong on two goldens, one honest gap, and extras that are real but front-loaded with cosmetics.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
5 high, 3 medium, 4 low 1 mitigated

This MR converts app-wide scale-color($primary, $lightness: X%) usages into dark-light-choose(scale-color($primary, ...), scale-color($secondary, ...)) so that dark themes derive their lightness from $secondary, using the established complement convention (X → 100−X). The pattern is applied consistently and correctly across the majority of the reviewed stylesheets (base, login, topic, topic-post, upload, admin, search, badges/buttons), and the changes preserve the existing light-theme rendering while making dark themes readable.

The review is not yet ready to merge. Several concrete gaps and regressions were found:

  • Regressions where the light-theme $primary lightness was unintentionally changed while adding the dark arm: the embedded-post author link in desktop/topic-post.scss (30% → 70%), the topic-map h3 in mobile/topic-post.scss (20% → 50%), and .group-member-info .name in both desktop/user.scss and mobile/user.scss (30% → 50%). These also break cross-layer consistency with their common/desktop counterparts. The original light values must be preserved.
  • Incomplete conversions leaving dark-theme support missing: the category badge in common/base/_topic-list.scss and the edit-category badge list link in mobile/modal.scss still hardcode $primary/#888.
  • Inconsistencies between desktop and mobile in mobile/modal.scss (custom-message-length lightness inverted vs desktop) and in the mobile composer status text/disabled-input surfaces.
  • Minor trailing-whitespace nits in desktop/topic-post.scss and desktop/queued-posts.scss, plus a low-severity contrast/polish note on the badge-notification text color.

All targeted files were reviewed in full; no functional, security, or variable-resolution problems were found, and dark-light-choose/$primary/$secondary resolve correctly in all consuming contexts. The remaining issues are limited to the specific lines flagged below.

These might need a close look

  • 🔴 app/assets/stylesheets/mobile/modal.scss (L102)
    .custom-message-length uses dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)), which is inverted relative to the desktop counterpart (desktop/modal.scss line 94, 70%/30%). This both regresses the light theme (returns the darker 30% value instead of the original 70%) and breaks desktop/mobile consistency. Use dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%)).
  • 🔴 app/assets/stylesheets/desktop/topic-post.scss (L291)
    The embedded-post author link's light-theme lightness was unintentionally changed from 30% to 70% while adding the dark arm (dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 30%))). This is a light-theme visual regression and makes the dark value inconsistent with other embedded-posts links. Preserve the original light value: dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)).
  • 🔴 app/assets/stylesheets/mobile/topic-post.scss (L182)
    The topic-map h3 light-theme lightness was unintentionally changed from 20% to 50% while adding the dark arm, both a light-theme regression and inconsistent with desktop/topic-post.scss (line 321, which correctly keeps 20%/80%). Use dark-light-choose(scale-color($primary, $lightness: 20%), scale-color($secondary, $lightness: 80%)).
  • 🔴 app/assets/stylesheets/desktop/user.scss (L522)
    .group-member-info .name light-theme lightness was silently changed from 30% to 50% during conversion, causing a light-theme regression and inconsistency with common/base/user.scss (which uses 30%/70%). Fix to dark-light-choose(scale-color($primary, $lightness: 30%), scale-color($secondary, $lightness: 70%)).
  • 🔴 app/assets/stylesheets/mobile/user.scss (L497)
    Same regression as desktop: .group-member-info .name light-theme lightness was changed from 30% to 50%, diverging from common/base/user.scss (30%/70%). Restore the 30% light value with the 70% dark counterpart.

Worth checking

  • 🟡 app/assets/stylesheets/mobile/modal.scss (L126)
    .edit-category-modal .secure-category-options .badge-list li a { color: #888; } was not converted, while its desktop counterpart (desktop/modal.scss line 116) was wrapped in dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)). The hardcoded #888 does not adapt to dark themes and diverges from desktop. Convert it (and its hover state) to the same dark-light-choose(50%, 50%) utility.
  • 🟡 app/assets/stylesheets/mobile/compose.scss (L36)
    The mobile composer status text (#draft-status line 36, .toggler line 51) now derives from scale-color($secondary, $lightness: 25%/50%) on dark themes, while the desktop composer leaves these elements at $primary. This creates a desktop/mobile dark-theme inconsistency and risks low/inverted contrast for #draft-status (~25% text on a ~40% dark surface). Confirm the intended dark look and consider a lighter $secondary lightness for these text elements.
  • 🟡 app/assets/stylesheets/common/base/_topic-list.scss (L115)
    The category-pills .badge-notification still uses color: scale-color($primary, $lightness: 50%) without a dark-theme $secondary arm, so on a dark theme it renders as a light-gray badge-on-dark, inconsistent with the rest of the topic list. Convert to dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)).
Small things (take or leave)
  • 🔵 app/assets/stylesheets/mobile/compose.scss (L168)
    Disabled composer inputs use a different dark mechanism than desktop: mobile #reply-title:disabled/.wmd-input:disabled use dark-light-choose(scale-color($primary, 75%), scale-color($secondary, 25%)) while desktop uses dark-light-diff($primary, $secondary, 90%, -60%). Both are dark-aware, so not a bug, but disabled inputs will render with different tint strength across breakpoints; consider aligning them. Minor.
  • 🔵 app/assets/stylesheets/desktop/topic-post.scss (L113)
    Several converted one-liners (lines 113, 294, 941) have a stray space before the closing brace (e.g. ...%)); }), inconsistent with the file convention (...%));}). Remove the trailing whitespace.
  • 🔵 app/assets/stylesheets/common/components/badges.css.scss (L239)
    The .badge-notification background was made theme-aware, but its color: $secondary (line 236) remains static, yielding low contrast in light (~2:1 on the light-gray chip) and dark (~3:1) themes. Pre-existing, but consider theming the color too (e.g. dark-light-choose(scale-color($primary, 50%), scale-color($secondary, 50%))) for consistency with header.scss. Minor polish, not blocking.
  • 🔵 app/assets/stylesheets/desktop/queued-posts.scss (L14)
    The rewritten rule span {color: dark-light-choose(...); } has a trailing space before the closing brace, inconsistent with surrounding formatting. The 50%/50% functional change is correct; just remove the stray whitespace.

✅ Feature-level checklist

Partially covered

  • ⚠️ scale-color $lightness must use $secondary for dark themes
    The pattern is correctly applied across most stylesheets (base/header, login, topic, topic-post, upload, admin, search, badges/buttons), with dark-light-choose(scale-color($primary, X%), scale-color($secondary, 100-X%)) returning the $secondary branch on dark themes. However, dark-theme support is incomplete: the category badge in common/base/_topic-list.scss:115 and the edit-category badge list link in mobile/modal.scss:126 remain on $primary/#888 with no $secondary arm. Additionally, several conversions (desktop/mobile user.scss, desktop/mobile topic-post.scss) silently altered the $primary light value rather than only adding the $secondary dark branch. These need to be corrected for the requirement to be fully satisfied.