Refactor theme settings and interfaces for improved structure and cla…#4551
Closed
teboho wants to merge 2 commits intoshesha-io:mainfrom
Closed
Refactor theme settings and interfaces for improved structure and cla…#4551teboho wants to merge 2 commits intoshesha-io:mainfrom
teboho wants to merge 2 commits intoshesha-io:mainfrom
Conversation
Contributor
WalkthroughBackend ThemeSettings was flattened and expanded with many new strongly-typed theme subtypes; the React theme provider/context gained matching TypeScript interfaces, extended initial state defaults, and re-exported the new types from the contexts module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-core/src/Shesha.Framework/Configuration/ThemeSettings.cs`:
- Line 41: Extract the repeated JSON literal used for StylingBox into a single
constant (e.g., private static readonly string DefaultStylingBox =
"{\"marginBottom\":\"0\"}") inside the ThemeSettings class and replace every
inline occurrence of the literal (all StylingBox initializers) with that
constant; update object initializers or defaults that currently set StylingBox =
"{\"marginBottom\":\"0\"}" to StylingBox = DefaultStylingBox (or use a public
const if the value should be exposed).
In `@shesha-reactjs/src/providers/theme/contexts.ts`:
- Line 80: The union type declared as application?: IApplicationTheme | Theme on
the provider context creates an untagged ambiguity (IApplicationTheme lacks
processingColor while Ant Design Theme includes it); update the types so
consumers don’t need defensive checks: either add a discriminator property
(e.g., kind: 'application' | 'antd-theme') to the union members and set it when
you create the context value, or extend/normalize IApplicationTheme to include
processingColor and convert the Ant Design Theme to that normalized shape at the
provider boundary (contexts.ts) so components like
src/components/colorPicker/index.tsx can safely read
theme.application.processingColor without extra optional chaining.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
shesha-core/src/Shesha.Framework/Configuration/ThemeSettings.csshesha-reactjs/src/providers/theme/contexts.tsshesha-reactjs/src/providers/theme/index.tsx
| }, | ||
| LayoutComponents = new LayoutComponentsSettings() | ||
| { | ||
| StylingBox = "{\"marginBottom\":\"0\"}", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deduplicate repeated StylingBox default literal.
The same JSON literal appears multiple times. Extracting a constant avoids drift and typo regressions.
♻️ Suggested refactor
+ private const string ZeroMarginBottomStylingBox = "{\"marginBottom\":\"0\"}";
+
public static ThemeSettings Default
{
get
{
return defaultInstance ??= new ThemeSettings()
{
LayoutComponents = new LayoutComponentsSettings()
{
- StylingBox = "{\"marginBottom\":\"0\"}",
+ StylingBox = ZeroMarginBottomStylingBox,
GridGapHorizontal = 8,
GridGapVertical = 8,
Border = new BorderTheme()
{
Border = new BorderConfig()
@@
InputComponents = new InputComponentsSettings()
{
LabelSpan = 8,
ContentSpan = 16,
LabelColon = true,
- StylingBox = "{\"marginBottom\":\"0\"}"
+ StylingBox = ZeroMarginBottomStylingBox
},
StandardComponents = new StandardComponentsSettings()
{
- StylingBox = "{\"marginBottom\":\"0\"}"
+ StylingBox = ZeroMarginBottomStylingBox
},
InlineComponents = new InlineComponentsSettings()
{
- StylingBox = "{\"marginBottom\":\"0\"}"
+ StylingBox = ZeroMarginBottomStylingBox
}
};
}
}Also applies to: 75-75, 79-79, 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-core/src/Shesha.Framework/Configuration/ThemeSettings.cs` at line 41,
Extract the repeated JSON literal used for StylingBox into a single constant
(e.g., private static readonly string DefaultStylingBox =
"{\"marginBottom\":\"0\"}") inside the ThemeSettings class and replace every
inline occurrence of the literal (all StylingBox initializers) with that
constant; update object initializers or defaults that currently set StylingBox =
"{\"marginBottom\":\"0\"}" to StylingBox = DefaultStylingBox (or use a public
const if the value should be exposed).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shesha-reactjs/src/providers/theme/contexts.ts (1)
81-81: 🛠️ Refactor suggestion | 🟠 MajorRemove the untagged
applicationunion (or make it discriminated).Line 81 still defines
application?: IApplicationTheme | Themeas an untagged union. Even withprocessingColornow present, this keeps narrowing ambiguous for consumers. Prefer a single normalized type at this boundary, or a discriminator if both shapes must be retained.♻️ Proposed fix
export interface IConfigurableTheme { - application?: IApplicationTheme | Theme; + application?: IApplicationTheme; sidebar?: 'dark' | 'light'; sidebarBackground?: string; layoutBackground?: string;In Ant Design 5.27.6, what is the recommended public type for app theme colors, and is importing `Theme` from `antd/lib/config-provider/context` considered stable API usage?As per coding guidelines: "Avoid monolithic types; use discriminated unions with a discriminator property instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/providers/theme/contexts.ts` at line 81, The field application currently uses an untagged union (application?: IApplicationTheme | Theme) which is ambiguous for consumers; change it to a single normalized type (prefer IApplicationTheme) at this boundary or, if you must keep both shapes, convert to a discriminated union by adding a discriminator property (e.g., kind: "custom" | "antd") to the variants and update all uses to narrow on that discriminator; ensure the new normalized type includes processingColor and remove direct reliance on importing Theme from antd context so consumers use the stable IApplicationTheme shape (or the new discriminated union) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shesha-reactjs/src/providers/theme/contexts.ts`:
- Line 81: The field application currently uses an untagged union (application?:
IApplicationTheme | Theme) which is ambiguous for consumers; change it to a
single normalized type (prefer IApplicationTheme) at this boundary or, if you
must keep both shapes, convert to a discriminated union by adding a
discriminator property (e.g., kind: "custom" | "antd") to the variants and
update all uses to narrow on that discriminator; ensure the new normalized type
includes processingColor and remove direct reliance on importing Theme from antd
context so consumers use the stable IApplicationTheme shape (or the new
discriminated union) instead.
Contributor
Author
|
Defunct, will push new with Sizwe's changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…rity
This will support Sizwe's incoming theme changes
#3836
Summary by CodeRabbit
New Features
Refactor