Tshepo/f/drawer component#3197
Conversation
WalkthroughThis update refactors the styling system for the Drawer component by centralizing and renaming style-related properties. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DrawerComponent
participant ShaDrawer
participant StylesManager
User->>DrawerComponent: Configure Drawer (settings, styles, dimensions)
DrawerComponent->>StylesManager: Consolidate allStyles (headerStyles, footerStyles, backgroundStyles, stylingBoxAsCSS, dimensions)
DrawerComponent->>ShaDrawer: Pass consolidated styles and dimensions
ShaDrawer->>ShaDrawer: Apply styles (backgroundStyles, stylingBoxAsCSS, dimensions)
ShaDrawer-->>User: Render styled Drawer
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
shesha-reactjs/src/designer-components/drawer/settingsForm.ts (2)
608-914:⚠️ Potential issueProp path mismatch: UI writes
headerStyles.*but component still readsheaderStyle
settingsFormnow stores header styles underheaderStyles, whiledrawer.tsxexpectsheaderStyle. Unless a mapper converts between them, headers will render with default styling.Confirm the mapping layer or rename the prop in
drawer.tsx.
916-1220:⚠️ Potential issueSame issue for
footerStylesvsfooterStyle
footerStyleswritten here is never consumed. Align prop names across settings, state model and component.
🧹 Nitpick comments (3)
shesha-reactjs/src/designer-components/drawer/drawer.tsx (1)
161-175: Inconsistent source of dimensions & spread order risk
- You already have
dimensionsin scope – prefer it toprops.dimensions.styleis spread before padding overrides; anypadding*specified instylewill override the derived values.- wrapper: { - width: style?.width || props.dimensions?.width, - height: style?.height || props.dimensions?.height, + wrapper: { + width: style?.width ?? dimensions?.width, + height: style?.height ?? dimensions?.height, ...rest, },Re-ordering (spread
restfirst) protects explicitstyleoverrides if that’s desired.shesha-reactjs/src/designer-components/drawer/models.ts (1)
33-34: Remove legacybackgroundprop or rename tobackgroundStylesfor consistencyElsewhere (
ShaDrawer,index.tsx) the refactor renamed the prop tobackgroundStyles, but the model still exposesbackground. Keeping both will confuse consumers and future contributors.- background?: IBackgroundValue; + /** @deprecated – superseded by `allStyles.backgroundStyles` */ + // background?: IBackgroundValue;Either deprecate and later delete, or migrate existing data so only the new property remains.
shesha-reactjs/src/designer-components/drawer/index.tsx (1)
23-24:styleis destructured but never used – triggers linter errorsconst { allStyles, style, headerStyles, footerStyles, ...props } = model;Unless you intend to support a direct
styleoverride, drop it from the pattern:-const { allStyles, style, headerStyles, footerStyles, ...props } = model; +const { allStyles, headerStyles, footerStyles, ...props } = model;This keeps the bundle free of unused variables and passes
no-unused-vars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
shesha-reactjs/src/designer-components/drawer/drawer.tsx(4 hunks)shesha-reactjs/src/designer-components/drawer/index.tsx(3 hunks)shesha-reactjs/src/designer-components/drawer/models.ts(2 hunks)shesha-reactjs/src/designer-components/drawer/settingsForm.ts(30 hunks)shesha-reactjs/src/designer-components/drawer/utils.ts(1 hunks)
🔇 Additional comments (3)
shesha-reactjs/src/designer-components/drawer/utils.ts (1)
17-24: Shadow block lacks a type definitionYou’ve added a
shadowliteral but no TS interface / type for it. Down-stream consumers will receiveany, defeating type-safety.Consider:
export interface IShadowStyle { offsetX: number; offsetY: number; color: string; blurRadius: number; spreadRadius: number; }and change the function return type accordingly.
shesha-reactjs/src/designer-components/drawer/drawer.tsx (1)
154-156: Ant-DesignDrawerhas noheightprop
heightis accepted only whenplacementistop | bottom. Passing it un-conditionally may log warnings. Add a conditional:height={['top', 'bottom'].includes(placement as string) ? dimensions?.height : undefined}shesha-reactjs/src/designer-components/drawer/settingsForm.ts (1)
16-32: Replacingnanoid()with hard-coded IDs risks collisionsHard-coding UUID-like strings removes determinism benefits without eliminating collision risk.
If the goal is diff stability, consider:
- keep
nanoid(), but memoise per-session, or- enforce uniqueness through a central registry.
| style: '', | ||
| shadow: { | ||
| offsetX: 0, | ||
| offsetY: 0, | ||
| color: '#000', | ||
| blurRadius: 0, | ||
| spreadRadius: 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
style should default to an object, not an empty string
style is later treated as a CSSProperties object (e.g. it is spread into content in drawer.tsx). Initialising it as '' breaks that contract and may trigger runtime errors when a string is spread.
- style: '',
+ style: {},📝 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.
| style: '', | |
| shadow: { | |
| offsetX: 0, | |
| offsetY: 0, | |
| color: '#000', | |
| blurRadius: 0, | |
| spreadRadius: 0 | |
| } | |
| style: {}, | |
| shadow: { | |
| offsetX: 0, | |
| offsetY: 0, | |
| color: '#000', | |
| blurRadius: 0, | |
| spreadRadius: 0 | |
| } |
| backgroundStyles?: CSSProperties; | ||
| dimensions?: { | ||
| width?: string | number; | ||
| height?: string | number; | ||
| }; | ||
| stylingBoxAsCSS?: CSSProperties; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Old & new props co-exist ⇒ API ambiguity
width (legacy) and dimensions.width (new) now sit side-by-side.
Keeping both will confuse users and complicate maintenance. Deprecate one path explicitly (JSDoc or runtime warning) and remove it in the next major.
| const { paddingTop, paddingRight, paddingBottom, paddingLeft, ...rest } = stylingBoxAsCSS; | ||
|
|
There was a problem hiding this comment.
Possible destructuring crash when stylingBoxAsCSS is undefined
If the prop is omitted, destructuring throws. Guard with a default:
- const { paddingTop, paddingRight, paddingBottom, paddingLeft, ...rest } = stylingBoxAsCSS;
+ const {
+ paddingTop,
+ paddingRight,
+ paddingBottom,
+ paddingLeft,
+ ...rest
+ } = stylingBoxAsCSS ?? {};📝 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.
| const { paddingTop, paddingRight, paddingBottom, paddingLeft, ...rest } = stylingBoxAsCSS; | |
| const { | |
| paddingTop, | |
| paddingRight, | |
| paddingBottom, | |
| paddingLeft, | |
| ...rest | |
| } = stylingBoxAsCSS ?? {}; |
| headerStyles?: IDrawerProps; | ||
| footerStyles?: any; | ||
| background?: IBackgroundValue; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid self-referential type explosion & restore type-safety
headerStyles recursively points to the entire IDrawerProps interface and footerStyles is typed as any.
This opens two problems:
- Infinite depth – any JSON–serialisation / deep-clone util walking the object tree could hit an endless loop or stack overflow because every
headerStylesagain contains its ownheaderStyles, and so on. - Loss of compile-time guarantees for
footerStyles.
Introduce a dedicated, flat style interface (e.g. IDrawerSectionStyles) that only exposes the properties really needed by the header/footer. Then reference it in both places.
- headerStyles?: IDrawerProps;
- footerStyles?: any;
+ headerStyles?: IDrawerSectionStyles;
+ footerStyles?: IDrawerSectionStyles;Don’t forget to export the new interface and update usages in index.tsx, the migrator and the settings form.
📝 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.
| headerStyles?: IDrawerProps; | |
| footerStyles?: any; | |
| background?: IBackgroundValue; | |
| headerStyles?: IDrawerSectionStyles; | |
| footerStyles?: IDrawerSectionStyles; | |
| background?: IBackgroundValue; |
| const additionalStyles: CSSProperties = removeUndefinedProps({ | ||
| ...shadowStyles, | ||
| ...stylingBoxAsCSS, | ||
| ...borderStyles, | ||
| stylingBoxAsCSS, | ||
| ...jsStyle, | ||
| }); |
There was a problem hiding this comment.
stylingBoxAsCSS is passed twice and pollutes inline CSS
stylingBoxAsCSS is already forwarded as an explicit prop (line 62) yet it is also embedded inside the style object – thereby producing an invalid CSS property and risking React warnings.
-const additionalStyles: CSSProperties = removeUndefinedProps({
- ...shadowStyles,
- ...borderStyles,
- stylingBoxAsCSS,
- ...jsStyle,
-});
+const additionalStyles: CSSProperties = removeUndefinedProps({
+ ...shadowStyles,
+ ...borderStyles,
+ ...jsStyle,
+});A single source of truth avoids duplication and keeps the generated style object valid.
📝 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.
| const additionalStyles: CSSProperties = removeUndefinedProps({ | |
| ...shadowStyles, | |
| ...stylingBoxAsCSS, | |
| ...borderStyles, | |
| stylingBoxAsCSS, | |
| ...jsStyle, | |
| }); | |
| const additionalStyles: CSSProperties = removeUndefinedProps({ | |
| ...shadowStyles, | |
| ...borderStyles, | |
| ...jsStyle, | |
| }); |
| .add<IDrawerProps>(4, (prev) => ({ | ||
| ...migratePrevStyles(prev, defaultStyles()), | ||
| desktop: { ...migratePrevStyles(prev, defaultStyles()).desktop, ...initialStyle }, | ||
| tablet: { ...migratePrevStyles(prev, defaultStyles()).tablet, ...initialStyle }, | ||
| mobile: { ...migratePrevStyles(prev, defaultStyles()).mobile, ...initialStyle }, | ||
| desktop: { | ||
| ...migratePrevStyles(prev, defaultStyles()).desktop, | ||
| headerStyles: defaultStyles(), | ||
| footerStyles: defaultStyles(), | ||
| }, | ||
| tablet: { | ||
| ...migratePrevStyles(prev, defaultStyles()).tablet, | ||
| headerStyles: defaultStyles(), | ||
| footerStyles: defaultStyles(), | ||
| }, | ||
| mobile: { | ||
| ...migratePrevStyles(prev, defaultStyles()).mobile, | ||
| headerStyles: defaultStyles(), | ||
| footerStyles: defaultStyles(), | ||
| }, | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migrate only once to reduce computation & dodge subtle divergence
migratePrevStyles(prev, defaultStyles()) is executed four times, producing four independent objects that can diverge if defaultStyles() ever becomes non-deterministic (timestamp, random IDs, etc.). It is also wasteful.
- .add<IDrawerProps>(4, (prev) => ({
- ...migratePrevStyles(prev, defaultStyles()),
- desktop: {
- ...migratePrevStyles(prev, defaultStyles()).desktop,
+ .add<IDrawerProps>(4, (prev) => {
+ const migrated = migratePrevStyles(prev, defaultStyles());
+ return {
+ ...migrated,
+ desktop: {
+ ...migrated.desktop,
headerStyles: defaultStyles(),
footerStyles: defaultStyles(),
},
tablet: {
- ...migratePrevStyles(prev, defaultStyles()).tablet,
+ ...migrated.tablet,
headerStyles: defaultStyles(),
footerStyles: defaultStyles(),
},
mobile: {
- ...migratePrevStyles(prev, defaultStyles()).mobile,
+ ...migrated.mobile,
headerStyles: defaultStyles(),
footerStyles: defaultStyles(),
},
- },
- })),
+ };
+ }))This single evaluation improves performance and guarantees consistency across breakpoints.
📝 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.
| .add<IDrawerProps>(4, (prev) => ({ | |
| ...migratePrevStyles(prev, defaultStyles()), | |
| desktop: { ...migratePrevStyles(prev, defaultStyles()).desktop, ...initialStyle }, | |
| tablet: { ...migratePrevStyles(prev, defaultStyles()).tablet, ...initialStyle }, | |
| mobile: { ...migratePrevStyles(prev, defaultStyles()).mobile, ...initialStyle }, | |
| desktop: { | |
| ...migratePrevStyles(prev, defaultStyles()).desktop, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| tablet: { | |
| ...migratePrevStyles(prev, defaultStyles()).tablet, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| mobile: { | |
| ...migratePrevStyles(prev, defaultStyles()).mobile, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| })), | |
| .add<IDrawerProps>(4, (prev) => { | |
| const migrated = migratePrevStyles(prev, defaultStyles()); | |
| return { | |
| ...migrated, | |
| desktop: { | |
| ...migrated.desktop, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| tablet: { | |
| ...migrated.tablet, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| mobile: { | |
| ...migrated.mobile, | |
| headerStyles: defaultStyles(), | |
| footerStyles: defaultStyles(), | |
| }, | |
| }; | |
| })) |
…work into tshepo/f/drawer-component
2 participants
#3018
Summary by CodeRabbit