Thulasizwe/en/dropdown tag#3579
Merged
James-Baloyi merged 3 commits intoshesha-io:mainfrom Jul 17, 2025
Merged
Conversation
Contributor
WalkthroughThe updates forward Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dropdown
participant Select
User->>Dropdown: Provide props (including disabledValues, ignoredValues)
Dropdown->>Select: Forward props (now includes disabledValues, ignoredValues)
Select-->>User: Render dropdown with appropriate disabled/ignored options
sequenceDiagram
participant User
participant IconPicker
participant Button
participant ShaIcon
User->>IconPicker: Select icon
IconPicker->>Button: Render with type='text', size, and icon prop
Button->>ShaIcon: Render icon inside button
Button-->>User: Display clickable selected icon as a button
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:
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
shesha-reactjs/src/components/dropdown/dropdown.tsx(1 hunks)shesha-reactjs/src/components/iconPicker/index.tsx(1 hunks)shesha-reactjs/src/components/iconPicker/styles/styles.ts(1 hunks)shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx(1 hunks)shesha-reactjs/src/components/refListDropDown/reflistTag.tsx(1 hunks)shesha-reactjs/src/designer-components/dropdown/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
shesha-reactjs/src/designer-components/dropdown/index.tsx (4)
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Learnt from: teboho
PR: shesha-io/shesha-framework#3374
File: shesha-reactjs/src/generic-pages/settings/dynamic-theme/textsExample.tsx:13-15
Timestamp: 2025-06-10T11:52:51.462Z
Learning: In the Shesha framework's dynamic theme system, the "Global text" Typography element in textsExample.tsx is intentionally left without explicit styling to demonstrate automatic global text color application through the ConfigProvider's colorText token, while the "Default text" element uses explicit theme color styling to show the difference between global and default text color approaches.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
shesha-reactjs/src/components/iconPicker/styles/styles.ts (4)
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Learnt from: teboho
PR: shesha-io/shesha-framework#3374
File: shesha-reactjs/src/generic-pages/settings/dynamic-theme/textsExample.tsx:13-15
Timestamp: 2025-06-10T11:52:51.462Z
Learning: In the Shesha framework's dynamic theme system, the "Global text" Typography element in textsExample.tsx is intentionally left without explicit styling to demonstrate automatic global text color application through the ConfigProvider's colorText token, while the "Default text" element uses explicit theme color styling to show the difference between global and default text color approaches.
shesha-reactjs/src/components/dropdown/dropdown.tsx (4)
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:171-192
Timestamp: 2025-06-26T20:01:48.838Z
Learning: In shesha-reactjs chart settings forms, duplicate input configurations may be intentionally included as part of UI/UX experiments and should be flagged for review during UI/UX review phases rather than immediately removed as errors.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/chartControl.tsx:51-51
Timestamp: 2025-06-26T20:00:58.574Z
Learning: In shesha-reactjs chart components (ChartControl), filters are intentionally passed as props rather than obtained from context. This allows the query builder and other external components to dynamically update filters. The pattern is: most chart configuration comes from useChartDataStateContext(), but filters are passed as props from the chart factories (bar.tsx, line.tsx, pie.tsx, polarArea.tsx) for dynamic updates.
shesha-reactjs/src/components/refListDropDown/reflistTag.tsx (3)
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
shesha-reactjs/src/components/iconPicker/index.tsx (6)
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:1280-1280
Timestamp: 2025-06-26T19:59:22.872Z
Learning: In shesha-reactjs chart settings forms, the styling panels (border, shadow, background) should not include buttonType-based hidden conditions since charts are not button components. The border configuration should follow the standard component pattern without button-specific restrictions.
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Learnt from: teboho
PR: shesha-io/shesha-framework#3312
File: shesha-reactjs/src/hooks/formComponentHooks.ts:183-192
Timestamp: 2025-05-28T07:55:21.036Z
Learning: In shesha-reactjs formComponentHooks.ts, the background style initialization deliberately sets valid values from properties panel immediately to provide better UX, rather than waiting for the useDeepCompareEffect to run. This intentional pattern prevents a flash of no background and shows the background image immediately when the component renders.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/chartControl.tsx:51-51
Timestamp: 2025-06-26T20:00:58.574Z
Learning: In shesha-reactjs chart components (ChartControl), filters are intentionally passed as props rather than obtained from context. This allows the query builder and other external components to dynamically update filters. The pattern is: most chart configuration comes from useChartDataStateContext(), but filters are passed as props from the chart factories (bar.tsx, line.tsx, pie.tsx, polarArea.tsx) for dynamic updates.
Learnt from: teboho
PR: shesha-io/shesha-framework#3374
File: shesha-reactjs/src/generic-pages/settings/dynamic-theme/textsExample.tsx:13-15
Timestamp: 2025-06-10T11:52:51.462Z
Learning: In the Shesha framework's dynamic theme system, the "Global text" Typography element in textsExample.tsx is intentionally left without explicit styling to demonstrate automatic global text color application through the ConfigProvider's colorText token, while the "Default text" element uses explicit theme color styling to show the difference between global and default text color approaches.
Learnt from: teboho
PR: shesha-io/shesha-framework#3461
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:171-192
Timestamp: 2025-06-26T20:01:48.838Z
Learning: In shesha-reactjs chart settings forms, duplicate input configurations may be intentionally included as part of UI/UX experiments and should be flagged for review during UI/UX review phases rather than immediately removed as errors.
shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx (1)
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
🔇 Additional comments (4)
shesha-reactjs/src/components/refListDropDown/reflistTag.tsx (1)
28-28: LGTM! Enhanced tooltip behavior and content structure.The changes improve the tooltip functionality by:
- Explicitly setting the trigger to hover for consistent behavior
- Providing better structured tooltip content with label and description when
showItemNameis false- Maintaining existing behavior when
showItemNameis trueshesha-reactjs/src/components/dropdown/dropdown.tsx (1)
182-183: LGTM! Proper forwarding of disabled and ignored values to Select component.The change correctly forwards the
disabledValuesandignoredValuesprops to the underlying Select component, enabling consistent option disabling and filtering behavior across dropdown components.shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx (1)
74-79: LGTM! Improved disabling logic from index-based to value-based.The changes improve the
disableValuefunction by:
- Removing the unused
indexparameter for cleaner function signature- Changing the disabling logic to check
item.valueinstead of relying on item position- Making the disabling behavior more consistent and logical
This aligns well with the value-based approach used throughout the dropdown system.
shesha-reactjs/src/components/iconPicker/styles/styles.ts (1)
17-22: LGTM! Clean CSS implementation for transparent button styling.The CSS rules are properly scoped to
.sha-icon-picker-selected-icon .ant-btnand effectively make the button wrapper transparent while maintaining functionality. This supports the Button wrapper changes in the main component.
| value={value} | ||
| size={model?.size} | ||
| tagStyle={{ ...tagStyle, alignContent: 'center' }} | ||
| tagStyle={{ ...tagStyle, alignContent: 'center', justifyContent: tagStyle.textAlign }} |
Contributor
There was a problem hiding this comment.
Fix CSS property value mismatch between textAlign and justifyContent.
The direct assignment of tagStyle.textAlign to justifyContent can cause invalid CSS values. The textAlign property uses values like 'left', 'right', 'center', 'justify', while justifyContent expects 'flex-start', 'flex-end', 'center', 'space-between', etc.
Apply this diff to properly map the values:
- tagStyle={{ ...tagStyle, alignContent: 'center', justifyContent: tagStyle.textAlign }}
+ tagStyle={{ ...tagStyle, alignContent: 'center', justifyContent: tagStyle.textAlign === 'left' ? 'flex-start' : tagStyle.textAlign === 'right' ? 'flex-end' : tagStyle.textAlign }}Or create a helper function to handle the mapping:
const getJustifyContent = (textAlign: string) => {
switch (textAlign) {
case 'left': return 'flex-start';
case 'right': return 'flex-end';
case 'center': return 'center';
case 'justify': return 'space-between';
default: return 'center';
}
};🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/dropdown/index.tsx at line 66, the CSS
property justifyContent is incorrectly assigned the value of tagStyle.textAlign,
which can cause invalid CSS. Fix this by mapping textAlign values to appropriate
justifyContent values using a helper function like getJustifyContent that
converts 'left' to 'flex-start', 'right' to 'flex-end', 'center' to 'center',
and 'justify' to 'space-between'. Then replace justifyContent:
tagStyle.textAlign with justifyContent: getJustifyContent(tagStyle.textAlign).
Comment on lines
+145
to
157
| <Button | ||
| size={selectBtnSize} | ||
| className={styles.shaIconPicker} | ||
| iconName={localSelectedIcon} | ||
| {...props} | ||
| style={{ fontSize: iconSize || 24 }} | ||
| name={localSelectedIcon} | ||
| type='text' | ||
| icon={<ShaIcon | ||
| className={styles.shaIconPicker} | ||
| iconName={localSelectedIcon} | ||
| {...props} | ||
| style={{ fontSize: iconSize || 24 }} | ||
| name={localSelectedIcon} | ||
| //title={localSelectedIcon} removed to avoid it conflicting with icon tooltip | ||
| />} | ||
| /> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant className and add accessibility attributes.
The Button wrapper improves consistency and accessibility, but there are two issues:
-
Redundant className: Both the Button and ShaIcon have
className={styles.shaIconPicker}, which is redundant and could cause styling conflicts. -
Missing accessibility: The Button should have a
titleoraria-labelattribute for screen readers.
Apply this diff to fix the issues:
<Button
size={selectBtnSize}
- className={styles.shaIconPicker}
type='text'
+ title={localSelectedIcon}
+ aria-label={`Selected icon: ${localSelectedIcon}`}
icon={<ShaIcon
className={styles.shaIconPicker}
iconName={localSelectedIcon}
{...props}
style={{ fontSize: iconSize || 24 }}
name={localSelectedIcon}
- //title={localSelectedIcon} removed to avoid it conflicting with icon tooltip
/>}
/>📝 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.
Suggested change
| <Button | |
| size={selectBtnSize} | |
| className={styles.shaIconPicker} | |
| iconName={localSelectedIcon} | |
| {...props} | |
| style={{ fontSize: iconSize || 24 }} | |
| name={localSelectedIcon} | |
| type='text' | |
| icon={<ShaIcon | |
| className={styles.shaIconPicker} | |
| iconName={localSelectedIcon} | |
| {...props} | |
| style={{ fontSize: iconSize || 24 }} | |
| name={localSelectedIcon} | |
| //title={localSelectedIcon} removed to avoid it conflicting with icon tooltip | |
| />} | |
| /> | |
| <Button | |
| size={selectBtnSize} | |
| type='text' | |
| title={localSelectedIcon} | |
| aria-label={`Selected icon: ${localSelectedIcon}`} | |
| icon={<ShaIcon | |
| className={styles.shaIconPicker} | |
| iconName={localSelectedIcon} | |
| {...props} | |
| style={{ fontSize: iconSize || 24 }} | |
| name={localSelectedIcon} | |
| />} | |
| /> |
🤖 Prompt for AI Agents
In shesha-reactjs/src/components/iconPicker/index.tsx around lines 145 to 157,
remove the redundant className={styles.shaIconPicker} from the ShaIcon component
to avoid styling conflicts, keeping it only on the Button. Additionally, add an
accessibility attribute such as title or aria-label to the Button component to
improve screen reader support, using a descriptive label like the selected icon
name.
James-Baloyi
approved these changes
Jul 17, 2025
Merged
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
2 participants
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.
#3278
Summary by CodeRabbit
New Features
Style
Bug Fixes