Refactor theming system to use centralized Lime Elements CSS variables #3624
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change removes all dependencies on internal MDC theming and variables, replacing them with new or updated CSS custom properties prefixed with Changes
Sequence Diagram(s)sequenceDiagram
participant ConsumerApp
participant LimeElements
Note over ConsumerApp: Sets <br> --lime-primary-color <br> --lime-on-primary-color <br> (optionally on :root or body)
ConsumerApp->>LimeElements: Loads components
LimeElements->>LimeElements: Components read CSS variables
LimeElements->>LimeElements: Apply accent color and font size using new variables
LimeElements-->>ConsumerApp: Render themed UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3624/ |
Kiarokh
force-pushed
the
theme-cleanup-v2
branch
from
3624822 to
3f26ea1
Compare
Kiarokh
force-pushed
the
theme-cleanup-v2
branch
5 times, most recently
from
43f5499 to
0e52a2d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses theming improvements by introducing standardized CSS custom properties and adding comprehensive theming documentation. The changes aim to provide better theming capabilities while maintaining backward compatibility.
- Replaces legacy MDC theme variables with new standardized
--limel-theme-*properties throughout the codebase - Adds a new theming documentation file explaining font customization and accent color configuration
- Consolidates font size references to use a single
--limel-theme-default-font-sizevariable
Reviewed Changes
Copilot reviewed 70 out of 70 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/theming.md | New comprehensive theming documentation covering font family and accent color customization |
| src/style/_theme-color-variables.scss | Updated theme color variables with new standardized naming convention |
| src/global/core-styles.scss | Added default font size variable definition |
| Multiple component SCSS files | Replaced legacy MDC theme variables with new standardized properties |
| guides.ts | Added theming documentation to the navigation structure |
Comments suppressed due to low confidence (2)
src/theming.md:57
- Missing closing parenthesis in the
rgb()function. Should bergb(var(--color-gray-lighter))instead ofrgb(var(--color-gray-lighter).
--lime-on-primary-color: rgb(var(--color-gray-lighter); /* Text/icons on primary color */
src/components/switch/switch.tsx:142
- Extra semicolon inside the string value. The semicolon should be outside the string or removed. Should be
color: 'var(--lime-primary-color, var(--limel-theme-primary-color))'without the semicolon inside the quotes.
color: 'var(--lime-primary-color, var(--limel-theme-primary-color));',
1a6249e to
415eb25
Compare
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 40
🔭 Outside diff range comments (7)
src/components/breadcrumbs/breadcrumbs.scss (1)
12-14: Duplicate custom-property definition will shadow itself
--limel-breadcrumbs-gapis declared twice (Lines 12–13). One of them is redundant and risks confusion when inspecting computed styles.- --limel-breadcrumbs-gap: 0.75rem;src/components/dialog/dialog.scss (1)
90-96: Mirror the fallback strategy used elsewhereSame concern as in
collapsible-section.scss: relying on a single CSS-variable with no default risks rendering content in the browser default colour. Add the old contrast-based fallback.- color: var(--limel-theme-on-surface-color); + color: var(--limel-theme-on-surface-color, rgb(var(--contrast-900)));src/components/markdown/partial-styles/_tables.scss (1)
24-30: Guard table cell font size with a fallbackFor consistency with
_body-text.scss, safeguard the variable with the previous literal value.- font-size: var(--limel-theme-default-font-size); // 14px + font-size: var(--limel-theme-default-font-size, 0.875rem); // 14 pxsrc/components/switch/switch.tsx (1)
136-155: Return value should be wrapped in<Host>instead of a hard-coded arrayStencil recommends wrapping multiple top-level elements in its special
<Host>component instead of returning an array literal.src/components/form/row/row.scss (1)
72-85: Colour variables updated, but MDC typography variables remainLines 77 and 83 now use the new Lime theme colour tokens—great.
However, the adjacent font-size declarations still rely on deprecated MDC tokens (--mdc-typography-*). For complete removal of MDC dependencies (stated PR goal), consider migrating these as well, e.g.:- font-size: var(--mdc-typography-subtitle1-font-size, 0.875rem); + font-size: var(--limel-theme-default-font-size); ... - font-size: var(--mdc-typography-body2-font-size, 0.8125rem); + font-size: calc(var(--limel-theme-default-font-size) * 0.93);(or another ratio that keeps the visual hierarchy).
This keeps the file fully aligned with the new theming strategy.
src/components/table/table.scss (1)
3-3: Tabulator SCSS import path is outdatedAccording to earlier work (PR #3478) Tabulator 6 exposes its SCSS at
node_modules/tabulator-tables/dist/scss/tabulator.scss, while the oldsrc/scss/path vanished.Failing to update this will break CI for anyone on the new Tabulator version.
-@import '../../../node_modules/tabulator-tables/src/scss/tabulator.scss'; +@import '../../../node_modules/tabulator-tables/dist/scss/tabulator.scss';src/components/tab-bar/tab-bar.scss (1)
147-162: Colour-variable naming is inconsistent with other componentsMost components use
--limel-theme-text-primary-on-background-color, but here the default label colour is--limel-theme-on-surface-color. Keeping one canonical name reduces cognitive load and doc noise. Please unify or add an alias.
♻️ Duplicate comments (2)
src/components/date-picker/flatpickr-adapter/flatpickr-adapter.scss (1)
289-294: Duplicate primary-colour chain here as wellWill be solved automatically if the earlier “single source of truth” refactor is applied.
src/style/_theme-color-variables.scss (1)
24-35: RGBA written withrgb()– duplicate of earlier remark
rgb(var(--contrast-1700), 0.38)(and the other two occurrences) is still invalid. Use eitherrgba()or the modern slash syntaxrgb(<r g b> / <alpha>).
Same issue was flagged in previous review → not repeating details.
Kiarokh
force-pushed
the
theme-cleanup-v2
branch
2 times, most recently
from
6fcd224 to
70163a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (22)
src/components/markdown/partial-styles/_headings.scss (1)
21-23:h6still hard-coded – align with theme variable for full consistency
Same remark as in earlier review; please consider usingvar(--limel-theme-default-font-size)or introducing a dedicated size token.src/examples/example-event-printer.scss (1)
12-12: Inline comment still misleading
The font size is now driven by--limel-theme-default-font-size, but the trailing comment implies a hard-coded 14 px value. Update or drop the comment to avoid confusion.src/components/collapsible-section/collapsible-section.scss (1)
101-104: Consider component-level override or fallback
Using the global--limel-theme-on-surface-colordirectly means consumers must override that global token to tweak just the title color. A component-specific variable (e.g.,--collapsible-section-title-color) cascading from the theme token would provide finer control.src/components/chip-set/chip-set.scss (1)
157-161: Delimiter color hard-wired to global token
For finer theming control expose--chip-set-delimiter-colorthat falls back to--limel-theme-on-surface-colorinstead of hard-coding the global token.src/components/banner/banner.scss (1)
8-10: Fixedmin-heighthard-codes size again – expose via custom property
A previous review already flagged this exact concern; the issue is still present after the refactor.src/style/internal/codemirror-tooltip.scss (2)
7-11: Color fallback still differs from background fallback
Background‐color falls back to the public--lime-*token first, butcolordoes not. If this was intentional, feel free to ignore; otherwise, mirror the same fallback pattern for consistency.
14-16: Font-size still lacks a hard fallback
Previous feedback suggested adding a pixel/rem fallback in case the theme variable is missing. Re-evaluate whether you want that safeguard or keep relying on the root guarantee.src/components/notched-outline/notched-outline.scss (1)
180-186: Still missing public--lime-error-color/--lime-error-text-colorfallbacksPrevious review already highlighted this. Other components use
var(--lime-error-color, var(--limel-theme-error-color))(and the same for*-text-color) so end-users can override the error colour.
Please mirror that pattern here to keep theming consistent.Also applies to: 193-194
src/components/table/partial-styles/tabulator-paginator.scss (2)
40-42: Add public surface background fallback
background-colorshould first try--lime-surface-background-colorthen fall back to--limel-theme-surface-background-color, like the rest of the refactor.
85-86: Expose on-surface colour to consumersReplace the single internal variable with
var(--lime-on-surface-color, var(--limel-theme-on-surface-color))for both the arrow border (left-color) and background so users can theme the paginator.Also applies to: 95-95
src/components/slider/slider.scss (1)
45-47: Use theme font-size variable instead of hard-coded 13 pxSame issue noted earlier: replace
0.8125remwith
var(--limel-theme-default-font-size)for consistency with the rest of the PR.- .mdc-slider__value-indicator-text { - font-size: 0.8125rem; //13px - } + .mdc-slider__value-indicator-text { + font-size: var(--limel-theme-default-font-size); + }src/components/slider/partial-styles/_thumb.scss (1)
73-76: Add hard-coded fallback to prevent transparent thumb indicatorBoth the value-indicator arrow (Line 73-76) and bubble (Line 91-94) rely solely on CSS variables. If neither
--lime-primary-colornor--limel-theme-primary-coloris defined, the indicator becomes transparent and unreadable. Re-add a static fallback (e.g. teal default) as proposed earlier.border-top-color: var( --lime-primary-color, var(--limel-theme-primary-color), + rgb(var(--color-teal-default)) ); ... background-color: var( --lime-primary-color, var(--limel-theme-primary-color), + rgb(var(--color-teal-default)) );Also applies to: 91-94
src/components/list/list.scss (1)
259-265: Selected row still lacks on-primary text colourBackground switches to the primary colour but text keeps the default table colour, causing illegibility on dark themes. Please add the corresponding text override.
.mdc-deprecated-list-item.mdc-deprecated-list-item--selected { &:before { background-color: var( --lime-primary-color, var(--limel-theme-primary-color) ); opacity: 0.15; + } + + color: var( + --lime-on-primary-color, + var(--limel-theme-on-primary-color) ); }src/components/select/select.scss (2)
39-40: Provide pixel fallback for font sizeIf
core-styles.scssfails to load, the text collapses to0px. Keep a hard fallback just like other components.- font-size: var(--limel-theme-default-font-size); + font-size: var(--limel-theme-default-font-size, 0.875rem);
151-154:<limel-icon>needsfill, not onlycolor, for error stateSVG icons ignore the
colorproperty. Addfillso the error hue is visible.- .invalid-icon { - color: var(--limel-theme-error-text-color); - } + .invalid-icon { + color: var(--limel-theme-error-text-color); + fill: var(--limel-theme-error-text-color); + }src/components/table/table.scss (1)
30-36:--table-arrow-color--activeincompatible with downstreamrgb()usageThe variable now resolves to a complete colour, but
tabulator-arrowlater wraps it inrgb(var(--table-arrow-color--active)), producing invalid CSS (rgb(var(--lime-primary-color))).
Either store a channel triplet or drop thergb()wrapper at use-sites.--table-arrow-color--active: var( - --lime-primary-color, - var(--limel-theme-primary-color) + var(--lime-primary-color, var(--limel-theme-primary-color)) );src/components/date-picker/flatpickr-adapter/flatpickr-adapter.scss (2)
81-95: Define a single--flatpickr-primaryvariable instead of duplicating the fallback chainThe same primary colour chain appears in
border-color,color,background,box-shadow, etc. Define it once at:hostlevel and reference it to avoid divergence and save bytes.:host(limel-flatpickr-adapter) { + --flatpickr-primary: var( + --lime-primary-color, + var(--limel-theme-primary-color) + ); + --flatpickr-on-primary: var( + --lime-on-primary-color, + var(--limel-theme-on-primary-color) + ); ... - border-color: var(--lime-primary-color, var(--limel-theme-primary-color)); - color: var(--lime-on-primary-color, var(--limel-theme-on-primary-color)); - background: var(--lime-primary-color, var(--limel-theme-primary-color)); + border-color: var(--flatpickr-primary); + color: var(--flatpickr-on-primary); + background: var(--flatpickr-primary);Also applies to: 98-104, 108-112
154-155: Add hard fallback for time-input colour
--limel-theme-on-surface-colormay be missing in consumer CSS. Provide a static RGB fallback.- color: var(--limel-theme-on-surface-color); + color: var(--limel-theme-on-surface-color, rgb(var(--contrast-1100)));src/components/switch/switch.scss (1)
58-73: Repeated primary-colour chain already noted earlier
Same observation as in the previous review: consider introducing a local--limel-primary-resolvedto DRY this block.src/components/form/form.scss (1)
181-186: Hard-coded typography constants previously discussed
Earlier feedback about introducing theme tokens instead of scattered magic numbers still applies.Also applies to: 202-223, 236-269, 284-285
src/style/internal/shared_input-select-picker.scss (1)
244-246: Same nested-var()issue already flagged earlier
Thebox-shadowstill containsvar(--lime-primary-color, var(--limel-theme-primary-color)), which is invalid for the same reason as above. Please flatten it.src/style/_theme-color-variables.scss (1)
24-35:rgb()used with 4 parameters – should bergba()
rgb(var(--contrast-1700), 0.38)(and the similar 0.87 / 0.54 cases) is invalid CSS. Usergba()when supplying an alpha channel, or keep it opaque.---limel-theme-text-disabled-on-background-color: rgb( - var(--contrast-1700), - 0.38 -); +--limel-theme-text-disabled-on-background-color: rgba( + var(--contrast-1700), + 0.38 +);Same fix applies to the two other occurrences in this block.
| :host(limel-button[primary]) & { | ||
| &:not(:disabled) { | ||
| color: var(--mdc-theme-on-primary, rgb(var(--color-white))); | ||
| color: var( | ||
| --lime-on-primary-color, | ||
| var(--limel-theme-on-primary-color) | ||
| ); | ||
| background-color: var( | ||
| --mdc-theme-primary, | ||
| rgb(var(--color-teal-default)) | ||
| --lime-primary-color, | ||
| var(--limel-theme-primary-color) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Factor-out the repeated primary color fallback chain
The same var(--lime-primary-color, var(--limel-theme-primary-color)) / --lime-on-primary-color fallback appears three times. Defining a resolved variable once on :host and re-using it would avoid copy-paste drift and keep future edits atomic.
:host {
+ /* Centralise resolved theme colours */
+ --limel-primary-resolved: var(--lime-primary-color, var(--limel-theme-primary-color));
+ --limel-on-primary-resolved: var(--lime-on-primary-color, var(--limel-theme-on-primary-color));
}
…
- color: var(--lime-on-primary-color, var(--limel-theme-on-primary-color));
+ color: var(--limel-on-primary-resolved);
…
- background-color: var(--lime-primary-color, var(--limel-theme-primary-color));
+ background-color: var(--limel-primary-resolved);
…
- color: var(--lime-primary-color, var(--limel-theme-primary-color));
+ color: var(--limel-primary-resolved);
…
- border-color: var(--lime-primary-color, var(--limel-theme-primary-color));
+ border-color: var(--limel-primary-resolved);Also applies to: 73-73, 147-147
🤖 Prompt for AI Agents
In src/components/button/button.scss around lines 55 to 65, the fallback color
variables for primary color and on-primary color are repeated multiple times. To
fix this, define CSS custom properties on the :host selector that resolve these
fallback chains once, then use these custom properties throughout the styles
instead of repeating the var() calls. Apply the same refactoring for the lines
73 and 147 as well to ensure consistency and easier maintenance.
| :host(.displays-percentage-colors[readonly]) { | ||
| --mdc-theme-on-surface: var(--mdc-theme-primary); | ||
| --mdc-theme-on-surface: var(--lime-primary-color); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Still exporting the deprecated MDC variable – switch to the new theme token
The declaration is now feeding Lime’s primary color into --mdc-theme-on-surface, i.e.
--mdc-theme-on-surface: var(--lime-primary-color);All other files in this PR delete the MDC namespace and use the new canonical variable --limel-theme-on-surface-color.
Keeping the MDC-prefixed name defeats the purpose of this refactor and leaks an alias that consumers are no longer supposed to rely on.
- --mdc-theme-on-surface: var(--lime-primary-color);
+ --limel-theme-on-surface-color: var(--lime-primary-color);🏁 Script executed:
#!/bin/bash
set -e
echo "=== Deprecated MDC variable usages ==="
rg -n -C2 "--mdc-theme-on-surface" --glob "*.scss" || echo "None found"
echo -e "\n=== New canonical theme variable usages ==="
rg -n -C2 "--limel-theme-on-surface-color" --glob "*.scss" || echo "None found"
echo -e "\n=== All CSS custom properties starting with --limel-theme- ==="
rg -o --no-filename "--limel-theme-[[:alnum:]-]*" --glob "*.scss" | sort -uLength of output: 1877
#!/usr/bin/env bash
set -euo pipefail
echo "=== Deprecated MDC variable usages ==="
rg --glob '*.scss' -- '--mdc-theme-on-surface' -n -C2 || echo "None found"
echo -e "\n=== New canonical theme variable usages ==="
rg --glob '*.scss' -- '--limel-theme-on-surface-color' -n -C2 || echo "None found"
echo -e "\n=== All CSS custom properties starting with --limel-theme- ==="
rg --glob '*.scss' -o --no-filename -- '--limel-theme-[[:alnum:]-]*' | sort -u || echo "None found"Switch to the new Lime Elements theme token
In this file we’re still defining the deprecated MDC property. To keep the codebase consistent and avoid leaking an unsupported alias, replace the old variable with the new canonical one.
Locations to update:
- File:
src/components/slider/partial-styles/percentage-color.scss
Lines: 7–9
Suggested change:
-:host(.displays-percentage-colors[readonly]) {
- --mdc-theme-on-surface: var(--lime-primary-color);
-}
+:host(.displays-percentage-colors[readonly]) {
+ --limel-theme-on-surface-color: var(--lime-primary-color);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :host(.displays-percentage-colors[readonly]) { | |
| --mdc-theme-on-surface: var(--mdc-theme-primary); | |
| --mdc-theme-on-surface: var(--lime-primary-color); | |
| } | |
| :host(.displays-percentage-colors[readonly]) { | |
| --limel-theme-on-surface-color: var(--lime-primary-color); | |
| } |
🤖 Prompt for AI Agents
In src/components/slider/partial-styles/percentage-color.scss at lines 7 to 9,
replace the deprecated CSS variable --mdc-theme-on-surface with the new Lime
Elements theme token variable. Update the variable name to the current canonical
one used in the Lime Elements theme to maintain consistency and avoid using
unsupported aliases.
583351d to
fd3d9d0
Compare
src/style/internal/lime-theme.scss
Outdated
| */ | ||
|
|
||
| @use '../brand-colors'; | ||
| @use '../_theme-color-variables'; |
There was a problem hiding this comment.
it affected limebb-navigation-button
now:
before:
There was a problem hiding this comment.
another miss of --mdc-theme-on-surface:
| } | ||
|
|
||
| &.mdc-chip--selected { | ||
| background-color: var(--mdc-theme-surface); |
There was a problem hiding this comment.
some icon missing it now:
|
|
||
| :host(limel-button[primary]) & { | ||
| &:not(:disabled) { | ||
| color: var(--mdc-theme-on-primary, rgb(var(--color-white))); |
There was a problem hiding this comment.
and this behaviour we could fix on go:
(it's not related to this pr)
Screen.Recording.2025-08-04.at.14.25.05.mov
There was a problem hiding this comment.
thanks for these 🙏 . I think I know why this happens. I'm gonna fix them soon
| @@ -1,4 +1,3 @@ | |||
| @use '../../style/internal/variables'; | |||
There was a problem hiding this comment.
It can be a wrong reference, please just check here
| &:disabled { | ||
| cursor: not-allowed; | ||
|
|
||
| color: var(--mdc-theme-text-disabled-on-background); |
This file included `@use '@material/theme'` which was supposed to add some of our very old brand colors to MDC components. However… 1. We're already overriding all MDC colors anyways. 2. There were some build errors related to this line, where the build couldn't find the file to import.
The `_theme-color-variables.scss` file is used in two places: 1. `lime-theme.scss` (which is imported by MDC-using components) 2. `core-styles.scss` (which is imported globally) Both files include the same `mixin` `theme-color-variables.theme-color-variables` on the `:root` or `:host` level. `core-styles.scss` is imported globally and already includes the theme variables at the `:root` level. While `lime-theme.scss` is imported into some individual components that use MDC and includes the same theme variables on the `:host` level. Since the theme variables are already defined at the `:root` level in `core-styles.scss`, they should be available throughout the entire application. Therefore, including in all components. The duplication is unnecessary.
Kiarokh
force-pushed
the
theme-cleanup-v2
branch
from
fd3d9d0 to
c691b83
Compare
…y on MDC styles Many of our components use MDC's custom CSS properties such as `--mdc-theme-primary` to render their styles properly. This is while we have had an undocumented property called `--lime-primary-color` which was intended to be used for theming. Due to our implementation and some spaghetti code, our own props worked inconsistently. So even we used MDC's props for styling our components.
…ts styles There doesn't seem to be anything that these components used from the imported `lime-theme.scss` file. It seemed that the file only includes some SASS variables (from us and from MDC), but none of them is used in the actual files. Comparing the compiled version of a couple of component's styles, I could see that the code was identical. Visual inspection also doesn't show any difference, and there is no build problems. Therefore, I just removed the imported file.
Since this file no longer defines or forwards any theme variables and instead only imports and forwards a typography file, we can rather only import the typography file where it is necessary.
And also remove the no-op `@use '@material/typography'`. We didn't really have any use for this file, except in one location. We handle typographic styles individually in components.
There was only one instance where something from this file was actually used. That is now handled locally. This file has otherwise nothing useful to offer.
Kiarokh
force-pushed
the
theme-cleanup-v2
branch
from
46ee3b5 to
a40d70e
Compare
|
🎉 This PR is included in version 38.21.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
fix #3623
Summary by CodeRabbit
New Features
Style
Documentation
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: