Thulasizwe/en/read only style#3655
Conversation
WalkthroughThis change refactors the logic and configuration for applying styles to components in read-only mode. It replaces the Changes
Sequence Diagram(s)sequenceDiagram
participant Designer as Designer UI
participant Component as Configurable Component
participant ReadOnlyItem as ReadOnlyDisplayFormItem
Designer->>Component: Set enableStyleOnReadonly (true/false)
Component->>Component: Check readOnly and enableStyleOnReadonly
alt enableStyleOnReadonly is false and readOnly
Component->>ReadOnlyItem: Render with minimal (font/dimension) style
else enableStyleOnReadonly is true or not readOnly
Component->>ReadOnlyItem: Render with full style
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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. 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: 5
🔭 Outside diff range comments (2)
shesha-reactjs/src/components/readOnlyDisplayFormItem/styles/styles.ts (1)
49-57: Ellipsis no longer works after commenting outwhite-space: nowrap
text-overflow: ellipsisneedswhite-space: nowrap(or multiline clamp) to take effect.
Removing it will cause text to wrap and the ellipsis will never appear.- // white-space: nowrap; + white-space: nowrap;shesha-reactjs/src/designer-components/dateField/datePickerWrapper.tsx (1)
241-241: Potential inconsistency in style application.The regular DatePicker still uses
allStyles.fullStyleinstead offinalStyle. This appears inconsistent with the pattern used for RangePicker and ReadOnlyDisplayFormItem in the same component.Consider updating to maintain consistency:
- style={allStyles.fullStyle} + style={finalStyle}
🧹 Nitpick comments (3)
shesha-reactjs/src/components/dropdown/model.ts (1)
53-54: Document the new flag and state the default
enableStyleOnReadonlychanges semantics compared to the deprecated flag but has no JSDoc or default-value note.
Please add a short description (what happens whentrue/false) and mention the implicit default (currentlyfalse).shesha-reactjs/src/designer-components/radio/utils.ts (1)
30-31: Add JSDoc forenableStyleOnReadonlyThe flag’s intent is not obvious—does
trueenable all styles or a subset?
Add a brief JSDoc and clarify default behaviour for consistency across components.shesha-reactjs/src/designer-components/numberField/numberField.tsx (1)
83-86: Good implementation of selective style application.The conditional style logic correctly preserves font and dimension styles while removing visual styling when
enableStyleOnReadonlyis false and the component is read-only.Consider using
useMemofor consistency with the textField implementation:+import React, { useMemo } from 'react'; -const finalStyle = !model.enableStyleOnReadonly && model.readOnly ? { - ...model.allStyles.fontStyles, - ...model.allStyles.dimensionsStyles, -} : model.allStyles.fullStyle; +const finalStyle = useMemo(() => !model.enableStyleOnReadonly && model.readOnly ? { + ...model.allStyles.fontStyles, + ...model.allStyles.dimensionsStyles, +} : model.allStyles.fullStyle, [model.enableStyleOnReadonly, model.readOnly, model.allStyles]);
| enableStyleOnReadonly?: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused prop detected
enableStyleOnReadonly is declared but never referenced inside CustomFile.
Either wire it into the rendering logic or drop it to avoid dead-code and API confusion.
🤖 Prompt for AI Agents
In shesha-reactjs/src/components/customFile/index.tsx around lines 31 to 32, the
prop enableStyleOnReadonly is declared but not used anywhere in the CustomFile
component. To fix this, either integrate enableStyleOnReadonly into the
component's rendering logic to apply conditional styling when the component is
readonly, or remove the prop declaration entirely if it is not needed, to
prevent dead code and reduce API confusion.
| danger={props.danger} | ||
| icon={props.icon ? <ShaIcon iconName={props.icon as IconType} /> : undefined} | ||
| iconPosition={props.iconPosition} | ||
| disabled={buttonDisabled} | ||
| className={classNames('sha-toolbar-btn sha-toolbar-btn-configurable', styles.configurableButton)} | ||
| className={classNames('sha-toolbar-btn sha-toolbar-btn-configurable', styles.configurableButton, buttonDisabled && styles.disabled)} | ||
| size={props?.size} |
There was a problem hiding this comment.
Keep the native disabled attribute for accessibility
Relying solely on a CSS class leaves the button focusable and the link navigable via keyboard, breaking expected disabled behaviour and accessibility contracts.
- className={classNames('sha-toolbar-btn sha-toolbar-btn-configurable', styles.configurableButton, buttonDisabled && styles.disabled)}
+ disabled={buttonDisabled}
+ className={classNames(
+ 'sha-toolbar-btn sha-toolbar-btn-configurable',
+ styles.configurableButton,
+ buttonDisabled && styles.disabled
+ )}This preserves visual styling while preventing interaction at the DOM level.
📝 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.
| danger={props.danger} | |
| icon={props.icon ? <ShaIcon iconName={props.icon as IconType} /> : undefined} | |
| iconPosition={props.iconPosition} | |
| disabled={buttonDisabled} | |
| className={classNames('sha-toolbar-btn sha-toolbar-btn-configurable', styles.configurableButton)} | |
| className={classNames('sha-toolbar-btn sha-toolbar-btn-configurable', styles.configurableButton, buttonDisabled && styles.disabled)} | |
| size={props?.size} | |
| danger={props.danger} | |
| icon={props.icon ? <ShaIcon iconName={props.icon as IconType} /> : undefined} | |
| iconPosition={props.iconPosition} | |
| disabled={buttonDisabled} | |
| className={classNames( | |
| 'sha-toolbar-btn sha-toolbar-btn-configurable', | |
| styles.configurableButton, | |
| buttonDisabled && styles.disabled | |
| )} | |
| size={props?.size} |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/button/configurableButton/index.tsx
around lines 80 to 84, the button is styled as disabled using a CSS class but
lacks the native disabled attribute, which breaks accessibility by allowing
keyboard focus and interaction. Add the native disabled attribute to the button
element, setting it based on the buttonDisabled condition, to prevent
interaction at the DOM level while preserving the visual disabled styling.
| const disabled = css` | ||
| cursor: not-allowed; | ||
| `; | ||
|
|
||
| return { | ||
| configurableButton | ||
| configurableButton, | ||
| disabled | ||
| }; |
There was a problem hiding this comment.
disabled class is only cosmetic – accessibility & events still active
Styling the cursor alone does not prevent clicks, keyboard activation, or convey the disabled state to assistive tech.
Ensure the consuming component either:
- Keeps the native
disabledattribute /aria-disabled="true", or - Adds
pointer-events: noneand blocksonClick.
Otherwise the button will look disabled but still fire actions.
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/button/configurableButton/style.ts
around lines 18 to 25, the disabled style only changes the cursor but does not
prevent interactions or convey the disabled state for accessibility. Update the
disabled style to include pointer-events: none to block mouse interactions, and
ensure the consuming component handles keyboard events and sets the native
disabled attribute or aria-disabled="true" to properly disable the button and
support assistive technologies.
| onChange={onChange} | ||
| placeholder={placeholder} | ||
| options={[...(options || [])].map(option => ({ ...option, label: <Icon icon={option.label} size={option.value} styles={styles} hint={tooltip} /> }))} | ||
| />; | ||
| />; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Icon size is set to option.value
option.value can be a string or non-numeric identifier, yet Icon expects a numeric size.
This causes runtime warnings and inconsistent icon sizing when the dropdown values are not numbers.
Consider either:
• Using a fixed numeric size, or
• Storing an explicit numeric iconSize on the option object.
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/inputComponent/index.tsx around lines
120 to 124, the Icon component's size prop is incorrectly set to option.value,
which may be non-numeric and cause runtime warnings. To fix this, replace
option.value with a fixed numeric size or add an explicit numeric iconSize
property to each option and use that for the Icon size prop to ensure consistent
and valid sizing.
| if (model.readOnly) { | ||
| return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} disabledStyleOnReadonly={model.disabledStyleOnReadonly} style={model.style} />; | ||
| return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} style={model.style} />; | ||
| } |
There was a problem hiding this comment.
Pass the new enableStyleOnReadonly flag
ReadOnlyDisplayFormItem now respects styles in read-only mode only when enableStyleOnReadonly is explicitly set.
Forward the model value so component styles are not silently dropped:
-return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} style={model.style} />;
+return (
+ <ReadOnlyDisplayFormItem
+ type="radiogroup"
+ disabled={model.readOnly}
+ render={renderCheckGroup}
+ style={model.style}
+ enableStyleOnReadonly={model.enableStyleOnReadonly}
+ />
+);📝 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.
| if (model.readOnly) { | |
| return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} disabledStyleOnReadonly={model.disabledStyleOnReadonly} style={model.style} />; | |
| return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} style={model.style} />; | |
| } | |
| if (model.readOnly) { | |
| - return <ReadOnlyDisplayFormItem type="radiogroup" disabled={model.readOnly} render={renderCheckGroup} style={model.style} />; | |
| + return ( | |
| + <ReadOnlyDisplayFormItem | |
| + type="radiogroup" | |
| + disabled={model.readOnly} | |
| + render={renderCheckGroup} | |
| + style={model.style} | |
| + enableStyleOnReadonly={model.enableStyleOnReadonly} | |
| + /> | |
| + ); | |
| } |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/radio/radioGroup.tsx around lines 66
to 68, the ReadOnlyDisplayFormItem component requires the enableStyleOnReadonly
flag to be set to apply styles in read-only mode. Update the JSX to pass
enableStyleOnReadonly={model.enableStyleOnReadonly} along with the existing
props to ensure styles are not silently dropped when the component is read-only.
1 participant
#3540
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation