-
Notifications
You must be signed in to change notification settings - Fork 2
new theme colors #201
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
new theme colors #201
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRestructures internal color tokens (introduces primary object and dedicated border object), updates many dark and light theme color values and SCSS variables, updates frontend asset hashes, and makes small component/type edits affecting theme usage and a props type plus an added optional loading callback invocation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/grafana-data/src/themes/createColors.ts (2)
109-113: Minor formatting inconsistency in border opacity values.Line 195 uses
0.4while line 112 (and the pattern in line 111) use0.40. For consistency, both should use the same notation (prefer0.40for clarity).- strong: `rgba(${this.blackBase}, .4)`, + strong: `rgba(${this.blackBase}, 0.40)`,Also applies to: 192-196
118-118: Fix whitespace in rgba color values.Lines 118 and 201 have unnecessary extra spacing in the rgba string before the final
1:
- Line 118 (DarkColors):
rgba(${this.whiteBase}, 1)(two spaces)- Line 201 (LightColors):
rgba(${this.blackBase}, 1)(two spaces)- contrastText: `rgba(${this.whiteBase}, 1)`, + contrastText: `rgba(${this.whiteBase}, 1)`,- contrastText: `rgba(${this.blackBase}, 1)`, + contrastText: `rgba(${this.blackBase}, 1)`,Also applies to: 201-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/grafana-data/src/themes/createColors.ts(3 hunks)public/microfrontends/fn_dashboard/index.html(1 hunks)public/sass/_variables.dark.generated.scss(10 hunks)public/sass/_variables.light.generated.scss(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: - Do not allow use ofeslint-disable,@ts-expect-error, or@ts-ignoreunless there's a clear, inline comment explaining why it's necessary.
- Suggest early returns in place of nested
if,elseor loops with complex branching.- Flag function-wide scopes created by
try/catchor top-levelif/else. Recommend moving the inner logic to its own function.- Flag use of
try/catchfor control flow. Recommend using.catch()with appropriate error handling.- Flag
try/catchthat introduces aletwhere.catch()withconstcould be used instead.- Flag
catchblocks that narrow the caughterrortoError. Suggest typing thecatchparameter asunknown.- Flag cases where types are narrowed manually before passing a value to the logger. Suggest passing the value directly without narrowing.
- Flag logging expressions that extract
error.messageor convert the error to a string. Suggest logging the full error value instead.- Flag variables created from
error.messageorString(error)that are then logged. Suggest logging the originalerrorvalue directly.- When
letis used to accumulate a value through conditions, suggest replacing it with a function that returns the final value directly.- Flag
letfollowed by a single conditional reassignment. Suggest aconstwith a ternary or a helper that returns the final value.- Flag Map get-or-create patterns that assign to a
let(e.g.let x = map.get(k); if (!x) { x = new ...; map.set(k, x) }). Suggest agetOrCreate*helper that returns the value and keeps the variableconst.- Flag nested conditions that can be combined into a single guard. Suggest simplifying with an early return.
- When encountering side effects such as mutation in
forEach, suggest replacing withmap,filterorreduce.- Recommend introducing intermediate variables when string interpolation contains non-trivial logic.
- Ban all
astype assertions everywhere, including chains like ...
Files:
packages/grafana-data/src/themes/createColors.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (9)
public/microfrontends/fn_dashboard/index.html (2)
10-22: Verify that nonce attributes are injected at build/runtime.Lines 10, 20, and 22 have empty
nonce=""attributes. If these are intended as Content Security Policy (CSP) placeholders filled in by the build or server, confirm the injection mechanism. Otherwise, the empty nonce provides no CSP protection and should either be removed or populated.
13-14: Asset hash updates look correct.The CSS and JavaScript asset references have been updated with new hash values, consistent with rebuilt frontend bundles for the new theme. No concerns with the path structure or references.
Also applies to: 22-22
packages/grafana-data/src/themes/createColors.ts (2)
95-99: Confirm primary object structure aligns with ThemeRichColor interface.The new
primaryobject structure (main, border, text) is introduced in both DarkColors and LightColors classes. Verify that this structure is compatible with the expectedThemeRichColorinterface shape, particularly in how it's consumed bygetRichColor()on lines 284–307.Also applies to: 178-182
93-171: Color system restructuring looks sound.The internal restructuring with the new primary object and dedicated border object is logically consistent. The color values across both DarkColors and LightColors follow a coherent pattern with proper theme inversions (light/dark text colors). The merge() composition at the end correctly combines base colors with getRichColor() enrichment.
public/sass/_variables.dark.generated.scss (2)
1-9: Verify source template was updated.This file is auto-generated from
grafana-ui/src/themes/dark.ts(as noted in the header). Ensure that the source template file was properly updated to regenerate these tokens. The extensive token updates throughout this file should be reviewed in the source template rather than in this generated artifact.
17-28: Token updates are consistent and comprehensive.The dark theme color tokens have been systematically updated across all categories (actions, layers, text, gradients, forms, panels, etc.) with appropriate light values for contrast against the dark backgrounds. The new color spectrum tokens (blue-light, blue-base, etc.) and updated brand/semantic colors align with the restructured color system in createColors.ts.
Also applies to: 64-117, 136-224
public/sass/_variables.light.generated.scss (3)
1-9: Verify source template was updated.This file is auto-generated from
grafana-ui/src/themes/light.ts(as noted in the header). Ensure that the source template file was properly updated to regenerate these tokens. The extensive token updates should be reviewed in the source template rather than in this generated artifact.
22-28: Clarify naming semantics for color tokens.The token names
$blue-light(#141116, very dark) and$variable(#141116, very dark) in the light theme are semantically confusing. These appear to be shadow/contrast values rather than actual "light" colors or "variables". Confirm this naming aligns with the design system intent and update token names or documentation if needed for clarity.Also applies to: 70-70, 75-75
60-62: Light theme token updates are consistent and comprehensive.The light theme color tokens have been systematically updated across all categories (layers, text, forms, panels, tooltips, popovers, etc.) with appropriate dark values for contrast against light backgrounds. The updates mirror the dark theme restructuring and align with the color system changes in createColors.ts.
Also applies to: 96-112, 131-174, 210-295, 341-344
92acbbf
into
coderabbit_micro_frontend
Summary by CodeRabbit
Style
Chores
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.