Repository navigation
Conversation
PR Summary by QodoAdd opt-in rebrand overrides for light, dark, and black themes
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1. Some light pages never receive the rebrand
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| body.light-app-theme.rebrand-theme { | ||
| --primary-color: #2b06e5; |
There was a problem hiding this comment.
2. Stock theme exports show rebrand colors 🐞 Bug ≡ Correctness
generate-colors.ts uses substring matching to classify the new compound selectors as ordinary light and dark theme rules, then places their declarations after the stock declarations. When @vibe/style builds, the published tokens.json can therefore report #2b06e5 as a stock theme’s primary color instead of #0073ea, even though the rebrand is opt-in.
Agent Prompt
## Issue description
The color generator treats opt-in rebrand selectors as stock light and dark theme selectors, corrupting the generated stock theme entries.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[9-10]
- packages/style/scripts/generate-colors.ts[82-95]
## Recommended Fix
Match stock theme selectors as complete selectors rather than substrings, and generate a separate rebrand entry if its colors need a JSON export. Verify generated stock primary colors remain `#0073ea`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| --placeholder-color: var(--secondary-text-color); | ||
| --icon-color: var(--secondary-text-color); | ||
| --disabled-background-color: #323230; | ||
| --link-color: #7e68ee; |
There was a problem hiding this comment.
3. Dark links lose readable contrast 🐞 Bug ≡ Correctness
The dark rebrand changes --link-color to #7e68ee, which has roughly 3.5:1 contrast against its #2a2a28 background, down from roughly 5.7:1 for the stock dark link color. Link components and anchors inside typography use this token for normal-size text, so their default state falls below the 4.5:1 text-contrast threshold when the theme is enabled.
Agent Prompt
## Issue description
The new dark link foreground has insufficient contrast against the new dark background for normal-size link text.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[94-94]
- packages/style/src/themes/rebrand-theme.scss[106-106]
- packages/components/link/src/Link/Link.module.scss[3-8]
## Recommended Fix
Choose a lighter dark-mode link color that retains the rebrand hue and reaches at least 4.5:1 contrast against the primary and secondary dark backgrounds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| --brand-color: #2b06e5; | ||
| --brand-hover-color: #2203be; | ||
| --brand-selected-color: rgba(255, 255, 255, 0.08); |
There was a problem hiding this comment.
2. Open brand split buttons lose readable text 🐞 Bug ≡ Correctness
The dark and black rebrand blocks set --brand-color to #2b06e5, which the split-button styles use for the secondary button’s text when an active tertiary brand button is open. With --brand-selected-color providing only an 8% white overlay on a dark background, that text has very low contrast in both variants.
Agent Prompt
## Issue description
Active tertiary brand split buttons use `--brand-color` as text when their secondary content is open, but the dark and black rebrand values have low contrast against the selected background.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[107-110]
- packages/style/src/themes/rebrand-theme.scss[175-178]
- packages/components/split-button/src/SplitButton/SplitButton.module.scss[48-65]
## Recommended Fix
Provide a lighter foreground token for brand text in this dark split-button state while preserving the intended blue CTA fill. Verify the active, open state in both dark and black themes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d63ff43 |
| --text-on-surface-neutral: var(--primary-text-color); | ||
| --allgrey-background-color: #f4f5f3; | ||
| --backdrop-color: rgba(50, 51, 56, 0.7); | ||
| --ui-border-color: #e2e4e3; |
There was a problem hiding this comment.
2. Idle light-theme controls lose their outlines 🐞 Bug ≡ Correctness
The light rebrand sets --ui-border-color to #e2e4e3, making default control borders barely distinguishable from their inherited white background. When the rebrand is enabled, unchecked checkboxes and unfocused text fields rely on that border to show their boundaries, while their hover and focus states use different colors.
Agent Prompt
## Issue description
The light rebrand's control border is too close to the white surface for idle controls to retain a clear outline.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[28-28]
## Recommended Fix
Choose a darker light-theme `--ui-border-color` that keeps default text fields, buttons, checkboxes, and radio buttons visibly bounded against their white backgrounds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit a4909c6 |
|
Code review by qodo was updated up to the latest commit bbd3223 |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
| body.dark-app-theme.rebrand-theme, | ||
| .rebrand-theme .dark-app-theme { | ||
| --primary-color: #4d63ff; | ||
| --primary-hover-color: #2b06e5; |
There was a problem hiding this comment.
2. Selected controls fade on dark hover 🐞 Bug ≡ Correctness
The dark and black rebrand set --primary-hover-color to #2b06e5, which selected checkboxes use as their fill and checked radios and slider thumbs use as their hover border. On either dark surface, those indicators have less than 3:1 contrast with their surroundings, so hovering or focusing a selected control makes its shape difficult to distinguish.
Agent Prompt
## Issue description
The dark and black button hover color is too dark to distinguish selected control fills and borders from their surroundings.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[63-64]
- packages/style/src/themes/rebrand-theme.scss[126-127]
- packages/components/checkbox/src/Checkbox/Checkbox.module.scss[4-7]
- packages/components/radio-button/src/RadioButton/RadioButton.module.scss[72-78]
- packages/components/slider/src/Slider/SliderBase/SliderThumb.module.scss[20-35]
## Recommended Fix
Give selected controls a contrasting hover color in the dark and black rebrand variants without changing the intended button hover color. Update the affected control styles to use the separate color and verify their fill and border contrast against both dark surfaces.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 95527a2 |
|
Code review by qodo was updated up to the latest commit 5d1f8bd |
Adds body.<theme>-app-theme.rebrand-theme override blocks (black logo, blue CTA), applied by the monolith behind design_facelift. Values are taken from mf-topbar's facelift theme (base block + black logo layer). generate-colors skips .rebrand-theme rules: their selectors contain the stock theme classes, so the substring match would merge the overrides into the stock entries of tokens.json. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d1f8bd to
4218c46
Compare
Mordech
changed the title
| --border-radius-full: 9999px; | ||
| } | ||
|
|
||
| body.dark-app-theme.rebrand-theme { |
There was a problem hiding this comment.
4. Nested themed panels revert to stock colors 🐞 Bug ≡ Correctness
The rebrand rules match themed bodies but not a theme-classed descendant of a rebranded body. A nested dark, light, or black subtree receives the stock theme declarations directly instead of the corresponding rebrand overrides.
Agent Prompt
## Issue description
Nested theme subtrees under a rebranded body use stock theme colors.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[7-7]
- packages/style/src/themes/rebrand-theme.scss[62-62]
- packages/style/src/themes/rebrand-theme.scss[117-117]
## Recommended Fix
Extend each override block with a `.rebrand-theme .{theme}-app-theme` selector so its variables apply directly to nested subtrees.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| body.light-app-theme.rebrand-theme { | ||
| --primary-color: #3543fc; |
There was a problem hiding this comment.
6. Consumers cannot obtain a logo color 🐞 Bug ≡ Correctness
The new light rebrand block does not define --logo-color, despite presenting black-logo styling as part of this layer. No stock theme defines that token either, so a consumer using the advertised variable receives no value, and the available logo SVG instead relies on an inherited currentColor.
Agent Prompt
## Issue description
The advertised logo-color token is absent, leaving consumers without a theme-controlled logo value.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[7-10]
- packages/style/src/themes/rebrand-theme.scss[62-69]
- packages/style/src/themes/rebrand-theme.scss[117-124]
## Recommended Fix
Define `--logo-color` in stock themes and override it with the intended black light-rebrand and white dark/black-rebrand values.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 4218c46 |
Synced with mf-topbar facelift.css (base + black logo layer): - Neutral hovers are the text color at 8%, selected-hover 15%; fill hovers are the on-color at 8% over the fill. On-secondary variants follow their base tokens. - Primary/brand/light link #424ef9 with white text; brand selected tints #424ef9 10% / 19%; dark/black positive and negative pass AA with white text, hover included. - RGB companions match the selected/highlight mixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| --positive-color: #007e23; | ||
| --positive-color-hover: color-mix(in srgb, var(--text-color-on-primary) 8%, var(--positive-color)); | ||
| --negative-color: #e70039; |
There was a problem hiding this comment.
4. Selected status buttons are hard to read 🐞 Bug ≡ Correctness
The dark and black rebrand blocks change --positive-color and --negative-color but leave their selected-background tokens at the stock theme values. When a secondary status button is selected, hovered, or focused, it displays the new foreground on an inherited dark surface; the positive pairing is approximately 1.6:1 contrast.
Agent Prompt
## Issue description
The dark and black rebrands change status foreground colors while retaining stock selected-state backgrounds, leaving selected secondary status buttons difficult to read.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[67-70]
- packages/style/src/themes/rebrand-theme.scss[126-129]
## Recommended Fix
Define selected and selected-hover status backgrounds in both rebrand blocks that remain distinguishable with their new foreground colors. Check the resulting secondary-button selected, hover, and focus states for both positive and negative variants.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit da30324 |
…emes The rebrand overrides introduce --surface-primary|positive|negative|warning| neutral and --border-radius-full. Define them in the stock light, dark, black and hacker themes (and :root for the radius) so they resolve without the rebrand class. Surfaces use the same formula, so each theme gets its own colours. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit dd976c7 |
Synced with mf-topbar facelift.css: - Placeholder: light #6c6b68, black #a9a5a1. - Dark/black: primary #6a86ff / #82a3ff, positive #3ca349 / #56bb5c, negative #ff4e60 / #ff7d8d, with --text-color-on-primary #201e24, so they pass 4.5:1 as small text on every surface and as fills (hover included). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| --negative-color-hover: color-mix(in srgb, var(--text-color-on-primary) 8%, var(--negative-color)); | ||
| --primary-color: #6a86ff; | ||
| --primary-hover-color: color-mix(in srgb, var(--text-color-on-primary) 8%, var(--primary-color)); | ||
| --text-color-on-primary: #201e24; |
There was a problem hiding this comment.
4. On-color buttons lose readable hover text 🐞 Bug ≡ Correctness
The dark and black rebrands use a dark --text-color-on-primary, but primary on-color buttons retain a hard-coded pale hover background while their text color is taken from the parent surface. When such a button sits directly on a primary-colored surface, hovering or focusing it places the new light-blue text on #e6e9ef, reducing contrast to approximately 2.7:1 in dark mode and 2.0:1 in black mode.
Agent Prompt
## Issue description
Dark and black rebrand on-color buttons can have light-blue text on a pale hover background, making their labels difficult to read.
## Fix Focus Areas
- packages/style/src/themes/rebrand-theme.scss[71-73]
- packages/style/src/themes/rebrand-theme.scss[130-132]
- packages/components/button/src/Button/Button.module.scss[191-200]
## Recommended Fix
Give the on-color primary button a theme-aware hover and focus background that preserves contrast with its parent-derived text color. Keep the stock themes' existing hover appearance unless intentionally changing it.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 3941a2f |
Synced with mf-topbar facelift.css: - --title-font-family: "monday Saans", Figtree, Roboto, ... in all three override blocks (body text stays Figtree). The host must also declare it where it sets its own fonts (DaPulse/dapulse#116378 does). - --border-radius-prompt-box: 12px. Not defined in the stock themes; consumers use var(--border-radius-prompt-box, 12px). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 4af2e62 |
12px on :root as well as in the rebrand overrides, so prompt boxes can migrate to the token before the flag is on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Same declarations, ordered under typography / primary & brand / semantic / interaction states / backgrounds / borders / text / rgb companions / surfaces / radius. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| --surface-primary: color-mix(in srgb, var(--primary-color) 8%, var(--primary-background-color)); | ||
| --surface-positive: color-mix(in srgb, var(--positive-color) 8%, var(--primary-background-color)); |
There was a problem hiding this comment.
4. New surfaces are absent from color docs 🐞 Bug ⚙ Maintainability
The new --surface-* declarations are not added to the shared or core color registries or to the semantic-colors page’s key list. When readers look up the newly available surface tokens in the color documentation, the page omits them, and description lookups for those names return no value.
Agent Prompt
## Issue description
The new semantic surface tokens exist in theme CSS but are absent from the color registries and semantic-colors documentation.
## Fix Focus Areas
- packages/style/src/themes/light-theme.scss[11-15]
- packages/shared/src/utils/colors-vars-map.ts[3-43]
- packages/core/src/utils/colors-vars-map.ts[1-43]
- packages/docs/src/pages/foundations/colors/semantic-colors/semantic-colors.tsx[4-24]
## Recommended Fix
Add all five surface tokens with descriptions to both color registries and include them in the semantic-colors page’s key list.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit a1ec2c3 |
--border-radius-full and --border-radius-prompt-box are now :root tokens, so the use-defined-css-var-when-available rule must know which props may use them. --border-radius-12 becomes the recommended var for a literal 12px now that --border-radius-prompt-box shares that value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 34a678c |
| --title-font-family: "monday Saans", Figtree, Roboto, Noto Sans Hebrew, Noto Kufi Arabic, Noto Sans JP, sans-serif; | ||
|
|
||
| /* Primary, brand and links */ | ||
| --primary-color: #424ef9; |
There was a problem hiding this comment.
@Mordech don't we want to give hard coded value a primitive value - like
--lilach-purple:#424ef9; and then use it ?
| /* Primary, brand and links */ | ||
| --primary-color: #424ef9; | ||
| --primary-hover-color: color-mix(in srgb, var(--text-color-on-primary) 8%, var(--primary-color)); | ||
| --text-color-on-primary: #ffffff; |
There was a problem hiding this comment.
same comment about whites ? give variable and then use it in the semantic tokens
| --primary-on-secondary-color: var(--primary-color); | ||
| --primary-hover-on-secondary-color: var(--primary-hover-color); | ||
| --link-on-secondary-color: var(--link-color); | ||
| --link-color: #424ef9; |
There was a problem hiding this comment.
same comment for all hard coded values
| "--border-radius-16" | ||
| "--border-radius-16", | ||
| "--border-radius-full", | ||
| "--border-radius-prompt-box" |
There was a problem hiding this comment.
Why not use the existing 12?
| --border-radius-medium: var(--border-radius-8); | ||
| --border-radius-big: var(--border-radius-16); | ||
| --border-radius-full: 9999px; | ||
| --border-radius-prompt-box: 12px; |
There was a problem hiding this comment.
Why not use theexisting 12?
|
Review: rebrand overrides The scoping looks right. Everything needs Accessibility The description says every text and fill pair is at least 4.5:1, but the PR has no script or test behind that. I computed the ratios from the hex values in the diff (hovers resolved by hand). Most pairs pass, with these exceptions:
Could we add a small script in Storybook There's no way to see this in Storybook. Tokens
Smaller things
|
Summary
Rebrand overrides (black logo, blue CTA) for the existing light, dark and black themes. This isn't a new theme: each block applies only when
<body>also hasrebrand-theme, which the monolith sets behinddesign_facelift.mf-topbar/src/styles/themes/facelift.css(base block +[data-facelift-logo=black]layer), limited to tokens that exist in Vibe. Primary/brand: light#424ef9with white text; dark#6a86ff/ black#82a3ffwith#201e24text.color-mix(in srgb, var(--text-color-on-primary) 8%, <fill>); brand selected =#424ef9at 10% / 19%. On-secondary variants follow their base tokens.--surface-primary|positive|negative|warning|neutral(color-mix(<color> 8%, var(--primary-background-color))) and--border-radius-full: 9999px. They're defined in the stock light/dark/black/hacker themes (and:rootfor the radius), and the overrides restate them with rebrand colours. Radius in the overrides: 6 / 8 / 12 / full 9999px.--title-font-family: "monday Saans", Figtree, Roboto, Noto Sans Hebrew, Noto Kufi Arabic, Noto Sans JP, sans-serifin all three blocks. Body text stays Figtree. The monolith declares--title-font-familyonbody#main.global-brand-font(an ID selector) which outranks this override, so DaPulse/dapulse#116378 restates it for.rebrand-themeand self-hosts the@font-face.--border-radius-prompt-box: 12pxon:root(border-radius.scss) and in the overrides, so consumers can migrate to the token before the flag; consumers usevar(--border-radius-prompt-box, 12px).generate-colors.tsskips.rebrand-themerules. Their selectors contain the stock theme classes, so without thistokens.jsonwould publish the override values as stock.Not included
--pulse-*,--facelift-*,--icon-hover).default-app-themeor a body with no theme class gets no override.Notes
--color-highlight_bluepoints atvar(--primary-highlighted-color). mf-topbar routes it through--primary-highlight-color, which isn't a Vibe token; the resolved value is the same.--fixed-light-color(white), which is fixed, so it stays below 4.5:1 on the lighter dark/black negative.Verification
sass src/index.scsscompiles.stylelintpasses.generate-colorsleavestokens.jsonunchanged.Consumed by DaPulse/dapulse#116378.
🤖 Generated with Claude Code