Skip to content

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

Open
zaibkhan wants to merge 1 commit into
theme-color-scheme-prefrom
theme-color-scheme-post
Open

scale-color $lightness must use $secondary for dark themes#4
zaibkhan wants to merge 1 commit into
theme-color-scheme-prefrom
theme-color-scheme-post

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Preserve light-theme lightness, fix one inversion
What’s good: Consistent use of dark-light-choose to map lightness to $secondary for dark themes, generally preserving light-theme appearance (e.g., 70/30, 75/25, 50/50).
Review Status: ✅ Safe to merge
Review Update:
• Scope: Large PR detected (32 files)
• Coverage: Reviewed 30 highest-risk files across 3 batches
• Note: For full confidence, consider splitting this PR; current review covers top 30/32 files

This review covered the top 30 of 32 files (risk-ranked). For complete coverage and faster feedback, consider splitting into ~1 smaller PR(s).

Issues (Medium)

Severity Issue Why it matters
Medium Correctness — Light-theme lightness inverted vs previous behavior …/mobile/compose.scss
This inverts the light-theme lightness compared to the previous value (70% → 30%) and also reverses the dark-theme complement relative to the pattern used elsewhere (expected 70/30, not 30/70). This will render the text noticeably darker on light theme than before.

Showing up to 1 medium issue(s). See inline suggestions for more details.

Key Feedback (click to expand)
  • Needs improvement: One instance in mobile/compose.scss flips the intended lightness (70%→30%) for the light theme; consider a small helper/mixin to encode the 70/30, 75/25 mapping to avoid drift.
  • Testing: Given the change is visual, consider a basic visual regression snapshot for light/dark themes around impacted components (e.g., custom-message-length, disabled buttons) to prevent lightness inversions in the future.
  • Documentation: If there’s a design guideline for mapping $primary lightness to $secondary in dark mode (e.g., 70%→30%, 75%→25%), document it near the theme functions or in a short style guide to reduce inconsistencies.
  • Compatibility: Ensure the dark-light-choose helper is available in all build paths including embed and mobile; also note that scale-color is deprecated in modern Sass—no action needed now, but plan a future migration to color.scale for long-term compatibility.
  • Performance: No material impact; all changes remain static SCSS resolution at compile time.
  • Open questions: Was the 30/70 split for .custom-message-length intentional to reduce emphasis on light theme, or should it align with the existing 70/30 pattern used elsewhere?

Confidence: 3/5 — Needs work before merge (1 medium · scope: top 30/32 files reviewed)

React with 👍 or 👎 if you found this review useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants