migration of display and wrap#3468
Merged
James-Baloyi merged 4 commits intoshesha-io:mainfrom Jun 27, 2025
Merged
Conversation
Contributor
WalkthroughThe Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsshesha-reactjs/src/designer-components/button/buttonGroup/utils.tsx (4)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
|
![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: 0
🧹 Nitpick comments (2)
shesha-reactjs/src/designer-components/container/data.ts (2)
203-203: Consider extracting the className logic for better maintainability.The conditional logic correctly sets
display: 'flex'for index table controls, but the hardcoded class names and repeated conditional checks could be extracted into a more maintainable pattern.Consider refactoring to reduce repetition:
+const isIndexTableControl = prev?.className === 'sha-index-table-control' || prev?.className === 'index-table-controls-right'; +const isRightControl = prev?.className === 'index-table-controls-right'; -display: prev?.className === 'sha-index-table-control' || prev?.className === 'index-table-controls-right' ? 'flex' : prev?.display ?? "block", +display: isIndexTableControl ? 'flex' : prev?.display ?? "block",
205-205: Logic is correct but reinforces the need for refactoring.Setting
flexWrap: 'nowrap'for index table controls is appropriate as these components typically need to keep items in a single line. This change works well with thedisplay: 'flex'override.This line uses the same conditional logic as line 203, which further supports extracting the className checks into variables for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/container/data.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
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/designer-components/container/data.ts (7)
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:1280-1280
Timestamp: 2025-06-26T19:59:22.839Z
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#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.814Z
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.493Z
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#3362
File: shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts:429-448
Timestamp: 2025-06-06T10:41:31.642Z
Learning: In shesha-reactjs chart settings, conditional visibility for controls is intentionally designed to show different combinations based on chart type: pie/polarArea charts show showTitle+showLegend together, while other chart types show showTitle separately. This pattern of "one shows when others doesn't show" is by design.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-attempt
🔇 Additional comments (1)
shesha-reactjs/src/designer-components/container/data.ts (1)
208-208: Good specificity for the alignItems override.The conditional logic appropriately targets only
'index-table-controls-right'for center alignment, which makes sense as right-aligned controls typically need vertical centering for proper visual alignment.The more specific targeting (only right controls vs. both control types) demonstrates thoughtful consideration of when centering is appropriate.
James-Baloyi
approved these changes
Jun 27, 2025
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.
#3354
Summary by CodeRabbit