Initial configuration for charts with responsiveness #3397
Conversation
|
""" WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChartControl
participant useProcessedChartData
participant ChartComponent
User->>ChartControl: Render with props
ChartControl->>useProcessedChartData: Fetch chart data (single call)
useProcessedChartData-->>ChartControl: Returns processed data
ChartControl->>ChartComponent: Render with responsive style and data
ChartComponent-->>User: Chart displayed with correct aspect ratio and config
Possibly related PRs
Suggested reviewers
Poem
📜 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 (1)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
shesha-reactjs/src/designer-components/charts/components/polarArea/index.tsx (1)
37-60:⚠️ Potential issue
scalesis misplaced insideplugins– Chart.js will ignore itIn Chart.js v3+,
scalesis a top-level option, not a plugin.
Currently the radial scale customisation never runs.-const options: ChartOptions<any> = { - responsive: true, - maintainAspectRatio: true, - aspectRatio: 1, - plugins: { - scales: { +const options: ChartOptions<'polarArea'> = { + responsive: true, + maintainAspectRatio: true, + aspectRatio: 1, + scales: { r: { ticks: { … }, grid: { … }, }, - }, - legend: { … }, - title: { … }, - }, + }, + plugins: { + legend: { … }, + title: { … }, + }, layout: { … }, };shesha-reactjs/src/designer-components/charts/components/line/index.tsx (1)
43-58: 🛠️ Refactor suggestionAvoid in-place mutation of props – clone
databefore decorating datasets.
useEffectdirectly mutatesdata.datasetsthat was received as a prop.
Although Chart.js tolerates the mutation, React best-practice is to treat props as immutable; future optimisations (memoisation, React 18 concurrent features, etc.) may produce subtle bugs when external libraries mutate objects that React components rely on for referential equality.- useEffect(() => { - if (dataMode === 'url') { - data?.datasets?.map((dataset: any) => { + useEffect(() => { + const clonedData = { ...data, datasets: data?.datasets?.map(ds => ({ ...ds })) }; + if (dataMode === 'url') { + clonedData.datasets?.forEach((dataset: any) => { ... }); - } else { - data?.datasets?.map((dataset: any) => { + } else { + clonedData.datasets?.forEach((dataset: any) => { ... }); } - }, [dataMode, data?.datasets, strokeColor, strokeWidth, tension]); + // propagate clonedData to the chart (e.g. keep it in state or pass down) + }, [dataMode, data, strokeColor, strokeWidth, tension]);Side-effect-free code simplifies reasoning and avoids hard-to-trace bugs when the same
datainstance is reused elsewhere.shesha-reactjs/src/designer-components/charts/hooks.ts (1)
85-89: 🛠️ Refactor suggestionDefault
borderColorof'#fff'makes line/bar outlines invisible on a white backgroundWhen
strokeColoris not supplied the fallback is white, causing all outlines (especially forlinecharts) to disappear.
Everywhere else in the PR the same fallback was switched to black (seecomponents/line/index.tsx). Keep the behaviour consistent and user-friendly.- borderColor: strokeColor || '#fff', + borderColor: strokeColor || 'black',
♻️ Duplicate comments (3)
shesha-reactjs/src/designer-components/charts/pie.tsx (1)
49-52:⚠️ Potential issueSame override issue as in LineChart migrator – swap spread order.
See comment in
charts/line.tsx; the pie chart migrator has identical precedence inversion.shesha-reactjs/src/designer-components/charts/polarArea.tsx (1)
49-52:⚠️ Potential issueSame override issue as in LineChart migrator – swap spread order.
See comment in
charts/line.tsx; the polar area chart migrator has identical precedence inversion.shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts (1)
433-436: Mirror clause may now contradict clause aboveBecause the first row shows when
(pie || polarArea || pivot)is true, the second row hides when the same expression is true.
If the previous comment is accepted, remember to revert this clause as well for symmetry.
🧹 Nitpick comments (5)
shesha-reactjs/src/designer-components/charts/utils.ts (1)
3-19: Type safety can be simplified & frozenThe explicit object-literal type is verbose and still allows mutation later.
Prefer an immutable “const assertion” and let TS infer the literal types:-export const defaultConfigFiller: { - showTitle: boolean; - simpleOrPivot: 'simple' | 'pivot'; - dataMode: TDataMode; - entityType: string; - axisProperty: string; - valueProperty: string; - aggregationMethod: TAggregationMethod; -} = { +export const defaultConfigFiller = { showTitle: true, simpleOrPivot: 'simple', dataMode: 'entityType' as TDataMode, entityType: 'Shesha.Domain.FormConfiguration', axisProperty: 'versionStatus', valueProperty: 'id', aggregationMethod: 'count' as TAggregationMethod, -}; +} as const;Benefits: shorter code, compile-time read-only, and literal narrowing without extra repetition.
shesha-reactjs/src/designer-components/charts/components/line/index.tsx (1)
63-65: Hard-coded aspect ratio may not fit all use-cases.
maintainAspectRatio: true&aspectRatio: 2imposes a global 2 : 1 ratio.
Consider exposing the ratio via props / context (with a sensible default) so consumers can opt-into taller or square charts without forking the component.shesha-reactjs/src/designer-components/charts/components/pie/index.tsx (1)
75-77: Parameterise aspect ratio instead of fixing it to 1.As with the line chart, fixing
aspectRatio: 1limits flexibility (e.g. wide dashboards).
Providing a prop or context value keeps the default while allowing overrides.shesha-reactjs/src/designer-components/charts/chartControl.tsx (2)
136-150:getResponsiveStyle– clamp ranges are asymmetric and may exceed viewport
- Width fallback ends at
100%, height ends at600px. Width can therefore stretch beyond the «95 vw» cap used in the explicit-size branch.- Comment says «800 px» but code clamps to
100%.Consider aligning caps and comments:
- : 'clamp(300px, 90vw, 100%)', + : 'clamp(300px, 90vw, 800px)',
162-175: Shadowingdataidentifier is confusing
const datainside the IIFE shadows the earlierdatain outer scope (state.data). Rename for clarity:- const data: IChartData = ... + const chartDataToRender: IChartData = ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
shesha-reactjs/src/designer-components/charts/bar.tsx(2 hunks)shesha-reactjs/src/designer-components/charts/chartControl.tsx(2 hunks)shesha-reactjs/src/designer-components/charts/components/bar/index.tsx(1 hunks)shesha-reactjs/src/designer-components/charts/components/line/index.tsx(1 hunks)shesha-reactjs/src/designer-components/charts/components/pie/index.tsx(1 hunks)shesha-reactjs/src/designer-components/charts/components/polarArea/index.tsx(1 hunks)shesha-reactjs/src/designer-components/charts/hooks.ts(1 hunks)shesha-reactjs/src/designer-components/charts/line.tsx(2 hunks)shesha-reactjs/src/designer-components/charts/pie.tsx(2 hunks)shesha-reactjs/src/designer-components/charts/polarArea.tsx(2 hunks)shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts(2 hunks)shesha-reactjs/src/designer-components/charts/styles.ts(2 hunks)shesha-reactjs/src/designer-components/charts/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts (1)
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 (3)
shesha-reactjs/src/designer-components/charts/styles.ts (1)
16-67:❓ Verification inconclusive
Check Emotion/antd-style support for nested
@media& child selectors
antd-style(Emotion under the hood) does allow nested queries, but only when babel-plugin or the macro is configured.
If those aren’t in place, the nested@mediablocks and& > divselector will be dropped at runtime.Please verify the build setup; otherwise extract the queries to top-level keys.
🏁 Script executed:
#!/bin/bash echo "🔍 Checking Babel configuration for Emotion plugin support" # Inspect common Babel config files if present for cfg in .babelrc babel.config.js babel.config.ts; do if [ -f "$cfg" ]; then echo "---- Found $cfg ----" sed -n '1,200p' "$cfg" echo fi done # Search package.json for relevant Emotion/babel-plugin entries echo "---- Scanning package.json for Emotion plugins ----" rg "\"@emotion/babel-plugin\"" -A1 -C1 package.json || echo "No @emotion/babel-plugin found" rg "\"babel-plugin-emotion\"" -A1 -C1 package.json || echo "No babel-plugin-emotion found" rg "\"babel-plugin-macros\"" -A1 -C1 package.json || echo "No babel-plugin-macros found"Length of output: 1000
🏁 Script executed:
#!/bin/bash echo "🔍 Locating package.json" find . -maxdepth 4 -type f -name package.json || echo "No package.json found within 4 levels" echo echo "🔍 Locating Babel config files" find . -maxdepth 4 -type f \( -name ".babelrc" -o -name ".babelrc.js" -o -name "babel.config.js" -o -name "babel.config.ts" \) || echo "No Babel config found" echo echo "🔍 Locating Vite/Webpack config" find . -maxdepth 4 -type f \( -name "vite.config.ts" -o -name "vite.config.js" -o -name "webpack.config.ts" -o -name "webpack.config.js" \) || echo "No Vite/Webpack config found"Length of output: 525
🏁 Script executed:
#!/bin/bash echo "🔍 Locating package.json in shesha-reactjs" find shesha-reactjs -maxdepth 4 -type f -name package.json || echo "No package.json found under shesha-reactjs" echo echo "🔍 Locating Babel config in shesha-reactjs" find shesha-reactjs -maxdepth 4 -type f \( -name .babelrc -o -name .babelrc.js -o -name babel.config.js -o -name babel.config.ts \) || echo "No Babel config under shesha-reactjs" echo echo "🔍 Locating build config in shesha-reactjs (Vite/Webpack)" find shesha-reactjs -maxdepth 4 -type f \( -name vite.config.ts -o -name vite.config.js -o -name webpack.config.ts -o -name webpack.config.js \) || echo "No build config under shesha-reactjs"Length of output: 1087
Verify Emotion nested styling support in
responsiveChartContainer(styles.ts)
Antd-style (Emotion under the hood) only handles nested selectors (e.g.& > div) and media queries (@media (…)) when you’ve set up the Emotion Babel plugin or macro. Without@emotion/babel-plugin(or enabling Emotion viababel-plugin-macros), those nested rules will be dropped at runtime.Please check your build configuration for one of the following:
- A Babel config (
.babelrc,babel.config.js/.ts) listingbabel-plugin-emotionor@emotion/babel-plugin- A macro setup (
babel-plugin-macros) enabling Emotion’s nested syntax- CRA/Vite/Next.js plugin integration that injects Emotion support
If you don’t have nested support enabled, extract your
& > divstyles and@mediablocks into top-level keys underresponsiveChartContainer.shesha-reactjs/src/designer-components/charts/components/bar/index.tsx (1)
43-45: Aspect-ratio tweak looks goodAdding
maintainAspectRatio: truewith a 2:1 ratio is a sensible default for bar charts and aligns with the new responsive container.shesha-reactjs/src/designer-components/charts/settingsFormIndividual.ts (1)
403-406: New pivot condition flips the original UX contract
pivotcharts are now treated likepie/polarArea, showing both «Show Title» and «Show Legend».
Previous learnings state that only pie-family charts expose both switches together, all other charts expose them separately. Double-check this product decision – it may unexpectedly hide the legend switch for pivot bar/line charts.If the original rule should stay, drop the pivot clause:
- || getSettingValue(data?.simpleOrPivot) === `pivot` + /* keep only pie & polarArea */
| .add<IChartProps>(5, prev => ({ | ||
| ...prev, | ||
| ...defaultConfigFiller, | ||
| })) |
There was a problem hiding this comment.
Order of spread overwrites user data – defaults should not trump existing values
{ ...prev, ...defaultConfigFiller } means any property already set by users in earlier versions (e.g. showLegend: false) gets unconditionally reset to the default (true).
Reverse the spread so only missing properties are filled:
-.add<IChartProps>(5, prev => ({
- ...prev,
- ...defaultConfigFiller,
-}))
+.add<IChartProps>(5, prev => ({
+ ...defaultConfigFiller,
+ ...prev,
+}))📝 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<IChartProps>(5, prev => ({ | |
| ...prev, | |
| ...defaultConfigFiller, | |
| })) | |
| .add<IChartProps>(5, prev => ({ | |
| ...defaultConfigFiller, | |
| ...prev, | |
| })) |
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/charts/bar.tsx around lines 49 to 52,
the spread order in the object merge causes defaultConfigFiller properties to
overwrite existing user settings in prev. To fix this, reverse the spread order
to { ...defaultConfigFiller, ...prev } so that user-defined properties in prev
take precedence and only missing properties are filled with defaults.
There was a problem hiding this comment.
The intention is to override.
There was a problem hiding this comment.
@teboho Got it—thanks for clarifying that the goal for version 5 is to reset existing props to the new defaults. I’ll keep that design choice in mind for future reviews. 🐇
✏️ Learnings added
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.548Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| .add<IChartProps>(5, prev => ({ | ||
| ...prev, | ||
| ...defaultConfigFiller, | ||
| })) |
There was a problem hiding this comment.
Migration step overrides existing user settings – swap spread order.
defaultConfigFiller is spread after prev, so any existing user-defined property is silently lost:
.add<IChartProps>(5, prev => ({
...prev,
...defaultConfigFiller, // <-- overrides prev
}))Invert the order to only fill missing fields:
- .add<IChartProps>(5, prev => ({
- ...prev,
- ...defaultConfigFiller,
- }))
+ .add<IChartProps>(5, prev => ({
+ ...defaultConfigFiller,
+ ...prev, // user settings win
+ }))Without this, migrating an existing form may unexpectedly reset options like showTitle or dataMode.
🤖 Prompt for AI Agents
In shesha-reactjs/src/designer-components/charts/line.tsx around lines 49 to 52,
the migration step spreads defaultConfigFiller after prev, which overwrites
existing user settings. To fix this, invert the spread order so that
defaultConfigFiller is spread first and then prev, ensuring that user-defined
properties in prev are preserved and only missing fields are filled by
defaultConfigFiller.
There was a problem hiding this comment.
The intention is to override.
There was a problem hiding this comment.
@teboho Got it—if the migration is meant to hard-reset those fields, then leaving defaultConfigFiller last is the correct approach. Just keep in mind that any forms already customised beyond the defaults will pick up the new baseline values after this migration; it might be worth flagging that in release notes or migration docs so teams don’t get surprised.
Let me know if there’s anything else you’d like me to check!
There was a problem hiding this comment.
actually I'm mistaken this makes sense
as suggested by code rabbit Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into split-charts
|
@James-Baloyi I made the adjustments, coderabbit was right on the migrations |
#3252
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Style