Apply required improvements to theme system of BlazorUI (#12303)#12304
Apply required improvements to theme system of BlazorUI (#12303)#12304msynk wants to merge 6 commits intobitfoundation:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughExpands theme data shapes (colors, shadows), adds color-role derivation, serialization, SSR bootstrap, presets, utilities for CSS-variable mapping/merging, refactors mapper to emit dark/light variants, updates manager/provider/js wiring, and adds contract/unit tests and demo docs. ChangesTheme System Improvements
Sequence DiagramsequenceDiagram
participant User as User/App
participant Provider as BitThemeProvider
participant Manager as BitThemeManager
participant Utilities as BitThemeUtilities
participant Mapper as BitThemeMapper
participant JS as JS Interop
User->>Provider: Set Theme & ParentTheme
Provider->>Utilities: Merge(Theme, ParentTheme)
Utilities->>Mapper: Merge(theme1, theme2)
Mapper-->>Utilities: merged BitTheme
Utilities-->>Provider: merged BitTheme
Provider->>Utilities: ToCssVariables(merged)
Utilities->>Mapper: MapToCssVariables(theme)
Mapper-->>Utilities: IReadOnlyDictionary<string, string>
Utilities-->>Provider: CSS variables dict
Provider->>Manager: ApplyBitThemeAsync(merged)
Manager->>Utilities: ToCssVariables(merged)
Utilities->>Mapper: MapToCssVariables(theme)
Mapper-->>Utilities: CSS variables
Utilities-->>Manager: CSS variables dict
Manager->>JS: BitThemeApplyBitTheme(variables, element)
JS->>JS: Set CSS custom properties on element
JS-->>Manager: Applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves the BlazorUI theme system by fixing theme merging behavior in BitThemeProvider, filling missing theme tokens (color variants + box shadows), and adding public utilities to map/merge/serialize themes—plus a small SSR helper and accompanying documentation/tests.
Changes:
- Fix
BitThemeProviderto emit CSS variables and cascade a merged theme (child overrides + parent fallback), rather than only the child theme. - Expand theme surface area (additional foreground/background/border variants and box-shadow tokens) and ensure they’re mapped/merged in
BitThemeMapper. - Add public helpers (
BitThemeUtilities,BitThemePresets,BitThemeSerialization,BitThemeColorDerivation,BitThemeSsr), update demo docs, and add contract-style unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cs | Adds mapper/merge/serialization contract tests, including a SCSS-driven “all referenced tokens are mapped” check. |
| src/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csproj | Copies theme-variables.scss into test output for the contract test. |
| src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/ThemingPage.razor | Documents precedence (preset vs inline vars) and introduces new utilities/SSR helper in the demo page. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeUtilities.cs | New public wrapper for mapping a BitTheme to CSS variables and merging themes. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSsr.cs | New SSR “early head script” helper for restoring/resolving bit-theme before CSS loads. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs | New JSON serialize/deserialize helpers for BitTheme. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeProvider.cs | Fixes merged-theme handling so inline CSS vars and cascaded value align. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemePresets.cs | Adds constants for well-known bit-theme preset names. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeMapper.cs | Adds missing CSS var mappings + merge support (general color variants and box shadows) and removes a duplicate typography block. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeManager.cs | Improves docs and routes Apply→CSS-vars mapping via BitThemeUtilities. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeJsExtensions.cs | Updates JS interop signature to accept IReadOnlyDictionary for theme variables. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeColorDerivation.cs | Adds helper to derive variant steps from a single main color without overwriting explicit values. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitTheme/BitThemeColors.cs | Expands BitThemeGeneralColorVariants with dark/light steps for primary/secondary/tertiary. |
| src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitTheme/BitThemeBoxShadows.cs | Adds missing box-shadow fields (callout2, sizes, inner) to the theme model. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs (1)
11-16: ⚡ Quick winConsider adding
PropertyNameCaseInsensitive = truefor robustnessWithout it, deserialization uses case-sensitive matching against the camelCase-transformed property names. JSON produced by a different serializer (e.g.
System.Text.Jsondefault /Newtonsoft.Jsondefault, which both emitPascalCase) will silently drop every field, returning a defaultBitTheme. The docs describe the use case as "sharing brand tokens", which implies possible interop with external producers.🛡️ Proposed fix
private static readonly JsonSerializerOptions Options = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, WriteIndented = true, DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + PropertyNameCaseInsensitive = true, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs` around lines 11 - 16, The JsonSerializerOptions defined in BitThemeSerialization (the static readonly Options) is currently case-sensitive and can drop properties when deserializing JSON with PascalCase names; update the Options object to set PropertyNameCaseInsensitive = true so deserialization will match property names regardless of casing (ensuring BitTheme deserialization works with external producers using PascalCase).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeColorDerivation.cs`:
- Around line 24-26: The light variant steps collapse because ScaleV uses
multiplicative scaling (ScaleV(v, 1.08/1.12/1.16)) causing high-v colors to
clamp to white; update BitThemeColorDerivation so the light variants
(variants.Light, variants.LightHover, variants.LightActive) use an additive
brightness offset instead of multiplicative scaling (e.g., replace ScaleV(v,
factor) calls with a method that returns ToHex(h, s, ClampV(v + delta)) or add
an AddV helper to compute v + fixedDelta and clamp to 1.0); keep referencing
ToHex, ScaleV (or new AddV/ClampV) and the variants.* properties so the steps
remain visually distinct for bright colors.
In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSsr.cs`:
- Around line 12-18: The SSR inline script constant InlineHeadScriptBody in
BitThemeSsr.cs doesn't honor the bit-theme-system attribute and so falls back to
light/dark before checking system preference; update the string in
InlineHeadScriptBody to first check if the document element has attribute
'bit-theme-system' (and if so resolve cur via matchMedia to dk/lt), then
fallback to persisted localStorage or bit-theme/bit-theme-default, keeping the
existing lt/dk retrieval and final r.setAttribute('bit-theme', cur) behavior so
server-side first paint matches the client helper logic.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/ThemingPage.razor`:
- Around line 281-284: The CodeBox example contains an escaped-at sequence that
produces invalid Razor: remove the extra escape before BitThemeSsr so the
expression reads as a normal cast; update the snippet in ThemingPage.razor (the
CodeBox showing @((MarkupString)...)) to use BitThemeSsr.InlineHeadScript
without the extra '@' escape so it renders as
@((MarkupString)BitThemeSsr.InlineHeadScript), not
@((MarkupString)@BitThemeSsr.InlineHeadScript).
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs`:
- Around line 11-16: The JsonSerializerOptions defined in BitThemeSerialization
(the static readonly Options) is currently case-sensitive and can drop
properties when deserializing JSON with PascalCase names; update the Options
object to set PropertyNameCaseInsensitive = true so deserialization will match
property names regardless of casing (ensuring BitTheme deserialization works
with external producers using PascalCase).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b36e58d0-269b-4e56-914d-e06688b68b65
📒 Files selected for processing (14)
src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitTheme/BitThemeBoxShadows.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitTheme/BitThemeColors.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeColorDerivation.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeJsExtensions.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeManager.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeMapper.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemePresets.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeProvider.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSsr.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeUtilities.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/ThemingPage.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cs
|
@coderabbitai re-review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
…m/msynk/bitframework into 12303-blazorui-theme-improvements
|
@coderabbitai re-review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeColorDerivationTests.cs (2)
211-219: 💤 Low valueDocument the luminance formula for maintainability.
The
Luminancehelper uses a simplified relative luminance calculation (ITU-R BT.601: 0.299R + 0.587G + 0.114B), which is appropriate for comparing relative brightness in tests. Adding a comment to clarify this would improve code maintainability and help future developers understand why this formula differs from the WCAG 2.0 gamma-corrected formula.📝 Suggested documentation
-/// <summary>Perceived luminance (0–1) from a `#RRGGBB` hex string.</summary> +/// <summary> +/// Simplified relative luminance (0–1) from a `#RRGGBB` hex string. +/// Uses ITU-R BT.601 formula (0.299R + 0.587G + 0.114B) for comparing +/// relative brightness. Note: This is not the WCAG 2.0 gamma-corrected formula. +/// </summary> private static double Luminance(string hex)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeColorDerivationTests.cs` around lines 211 - 219, Add a brief comment above the Luminance method documenting that the function uses the simplified ITU‑R BT.601 perceived luminance formula (0.299*R + 0.587*G + 0.114*B) returning a 0–1 value for comparative brightness in tests, and note that this is intentionally different from the WCAG 2.0 gamma‑corrected relative luminance; reference the formula name and why it’s used here for maintainability and future clarity for the Luminance(string hex) helper.
181-195: 💤 Low valueConsider testing the contrast threshold boundary.
The tests verify text color selection for dark and light base colors. Consider adding a test case near the luminance threshold where the implementation switches from white to black text to ensure the boundary behavior is correct.
💡 Example boundary test
+[TestMethod] +public void FillColorRoleFromMain_MediumBrightnessColor_TextContrastIsAppropriate() +{ + // Test a color near the threshold (adjust hex based on implementation threshold) + var v = new BitThemeColorVariants(); + BitThemeColorDerivation.FillColorRoleFromMain(v, "#808080"); // medium grey + // Assert appropriate contrast choice based on implementation + Assert.IsTrue(v.Text == "#FFFFFF" || v.Text == "#000000", "Text should be either white or black"); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeColorDerivationTests.cs` around lines 181 - 195, Add a boundary unit test that verifies FillColorRoleFromMain's contrast threshold behavior: create a new test (e.g., FillColorRoleFromMain_AtLuminanceThreshold_VerifiesTextColor) that constructs a BitThemeColorVariants, calls BitThemeColorDerivation.FillColorRoleFromMain with a hex base color chosen at or just around the luminance threshold (one value slightly darker and one slightly lighter), and Assert.AreEqual expected v.Text ("#FFFFFF" vs "#000000") for each to ensure the implementation switches text color at the intended boundary; reference BitThemeColorDerivation.FillColorRoleFromMain and BitThemeColorVariants when locating where to add the test.src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cs (1)
95-124: ⚡ Quick winUse reference equality for the visited set, matching production traversal.
new HashSet<object>()(created at the call site on line 29 via the empty collection expression) uses the default object equality. The production traversal inBitThemeSerialization.EnsureNestedObjectsdeliberately usesReferenceEqualityComparer.Instancebecause two distinct POCO instances can be value-equal (and any futureEquals/GetHashCodeoverride on aBitTheme*POCO would silently make this filler skip a real branch — under-filling the theme and turning theThemeVariablesReferencedTokensAreEmittedByMapperWhenSetcontract into a false negative). Easy to align here:♻️ Use ReferenceEqualityComparer for visited tracking
@@ var theme = new BitTheme(); - FillAllStringProperties(theme, []); + FillAllStringProperties(theme, new HashSet<object>(ReferenceEqualityComparer.Instance)); @@ - private static void FillAllStringProperties(object? obj, HashSet<object> visited) + private static void FillAllStringProperties(object? obj, HashSet<object> visited) { if (obj is null) return; if (!visited.Add(obj)) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cs` around lines 95 - 124, The visited set in FillAllStringProperties uses default object equality which can treat distinct POCO instances as equal if Equals/GetHashCode are overridden; change the caller (where FillAllStringProperties is invoked with new HashSet<object>()) to use a reference-equality set instead (e.g., HashSet<object>.CreateSetComparer or ReferenceEqualityComparer.Instance) so traversal matches BitThemeSerialization.EnsureNestedObjects; update the call site that constructs the HashSet<object> (used by the ThemeVariablesReferencedTokensAreEmittedByMapperWhenSet test) to pass a HashSet that uses reference equality to prevent skipping distinct instances.src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs (1)
16-26: 💤 Low valueOptional:
Options.WriteIndentedis dead configuration on the serialize path.The output is always re-serialized through
JsonNode.ToJsonString(JsonNodeWriteOptions)after pruning, soWriteIndented = trueonOptionsonly affects the intermediaterawstring that's immediately discarded. You can drop it fromOptionsto keep intent clear (or alternatively reuseOptionsfor both calls and removeJsonNodeWriteOptions).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs` around lines 16 - 26, The Options JsonSerializerOptions has WriteIndented = true but that setting is unused because the code immediately reparses/prunes and calls JsonNode.ToJsonString(JsonNodeWriteOptions); either remove WriteIndented from the Options initializer to reflect actual intent, or eliminate JsonNodeWriteOptions and reuse Options for the final JsonNode.ToJsonString call (and remove the separate JsonNodeWriteOptions field); update references to JsonNode.ToJsonString and the Options/JsonNodeWriteOptions fields accordingly so only the effective serializer options remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cs`:
- Around line 16-26: The Options JsonSerializerOptions has WriteIndented = true
but that setting is unused because the code immediately reparses/prunes and
calls JsonNode.ToJsonString(JsonNodeWriteOptions); either remove WriteIndented
from the Options initializer to reflect actual intent, or eliminate
JsonNodeWriteOptions and reuse Options for the final JsonNode.ToJsonString call
(and remove the separate JsonNodeWriteOptions field); update references to
JsonNode.ToJsonString and the Options/JsonNodeWriteOptions fields accordingly so
only the effective serializer options remain.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeColorDerivationTests.cs`:
- Around line 211-219: Add a brief comment above the Luminance method
documenting that the function uses the simplified ITU‑R BT.601 perceived
luminance formula (0.299*R + 0.587*G + 0.114*B) returning a 0–1 value for
comparative brightness in tests, and note that this is intentionally different
from the WCAG 2.0 gamma‑corrected relative luminance; reference the formula name
and why it’s used here for maintainability and future clarity for the
Luminance(string hex) helper.
- Around line 181-195: Add a boundary unit test that verifies
FillColorRoleFromMain's contrast threshold behavior: create a new test (e.g.,
FillColorRoleFromMain_AtLuminanceThreshold_VerifiesTextColor) that constructs a
BitThemeColorVariants, calls BitThemeColorDerivation.FillColorRoleFromMain with
a hex base color chosen at or just around the luminance threshold (one value
slightly darker and one slightly lighter), and Assert.AreEqual expected v.Text
("#FFFFFF" vs "#000000") for each to ensure the implementation switches text
color at the intended boundary; reference
BitThemeColorDerivation.FillColorRoleFromMain and BitThemeColorVariants when
locating where to add the test.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cs`:
- Around line 95-124: The visited set in FillAllStringProperties uses default
object equality which can treat distinct POCO instances as equal if
Equals/GetHashCode are overridden; change the caller (where
FillAllStringProperties is invoked with new HashSet<object>()) to use a
reference-equality set instead (e.g., HashSet<object>.CreateSetComparer or
ReferenceEqualityComparer.Instance) so traversal matches
BitThemeSerialization.EnsureNestedObjects; update the call site that constructs
the HashSet<object> (used by the
ThemeVariablesReferencedTokensAreEmittedByMapperWhenSet test) to pass a HashSet
that uses reference equality to prevent skipping distinct instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca2158c0-555c-4e0c-805a-81262e23c917
📒 Files selected for processing (8)
src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeColorDerivation.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeManager.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSerialization.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSsr.cssrc/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeUtilities.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeColorDerivationTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeMapperContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Utils/Theme/BitThemeSerializationTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeSsr.cs
- src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeColorDerivation.cs
- src/BlazorUI/Bit.BlazorUI/Utils/Theme/BitThemeUtilities.cs
Co-authored-by: Copilot <copilot@github.com>
|
@coderabbitai re-review |
|
✅ Actions performedReview triggered.
|
| public async ValueTask ApplyBitThemeAsync(BitTheme bitTheme, ElementReference? element = null) | ||
| /// <summary>Applies <paramref name="bitTheme"/> as CSS custom properties on <paramref name="element"/> (default: body), overriding stylesheet tokens for that subtree.</summary> | ||
| public async ValueTask ApplyBitThemeAsync(BitTheme? bitTheme, ElementReference? element = null) | ||
| { |
closes #12303
Summary by CodeRabbit