-
Notifications
You must be signed in to change notification settings - Fork 458
feat(configurator): add NestedObjectConfigurator for chart components and update chart bundle #1750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(configurator): add NestedObjectConfigurator for chart components and update chart bundle #1750
Conversation
…into feat/check-source-code
…into feat/chart-options-configurator
WalkthroughAdds a detailed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue`:
- Line 8: The template references an undefined variable via
:data-group-index="index" in ObjectDepthConfigurator.vue; add a prop named index
to the component's props (or define it in setup) or remove/replace the binding
with the correct existing value. Specifically, if ConfigItem consumers expect a
numeric group index, add an index prop (e.g., props: { index: { type: Number,
required: true } }) to the ObjectDepthConfigurator component and use that prop
in :data-group-index, otherwise change the attribute to use the correct existing
variable or omit it.
- Around line 27-30: The prop definition for meta in ObjectDepthConfigurator.vue
uses a default arrow function that returns undefined; update the meta prop's
default to return an empty object (e.g., change the default factory to return {}
instead of using a block with no return) so that accesses like
props.meta.properties or props.meta.widget won't throw; locate the meta prop in
the component options (meta: { type: Object, default: ... }) and replace the
default implementation with a function that returns an empty object.
- Around line 34-47: The computed property properties is mutating objects from
props.meta.properties[0].content (setting item.widget.props.modelValue) and also
accesses props.meta.widget.props?.modelValue without ensuring props.meta.widget
exists; change properties to produce a new array (use map) and deep-clone each
item before assigning modelValue so you don't mutate the original props, and
guard access to props.meta.widget (e.g., read
props.meta.widget?.props?.modelValue) when retrieving propsModelValue; update
references to the computed properties, props.meta.properties,
props.meta.widget.props.modelValue, and item.widget.props.modelValue while
making these changes.
- Around line 48-50: itemChange currently reads
props.meta.widget.props.modelValue without null checks which can throw if
meta/widget/props is undefined; update itemChange to safely access the nested
value (use optional chaining on props.meta?.widget?.props?.modelValue) and
default to an empty object before spreading, then emit the update as before
(reference itemChange and the emit('update:modelValue', ...) call).
🧹 Nitpick comments (4)
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue (1)
3-6: Redundant:keyon nested element.The
:key="idx"on line 5 is redundant since the parentdivalready has:key="idx". The key is only needed on the element withv-for.Proposed fix
<div v-for="(data, idx) in properties" :key="idx" class="meta-config-item"> <config-item - :key="idx" :property="data" :data-prop-index="idx"designer-demo/public/mock/bundle.json (3)
4115-4130: Extraneouslanguageprop in SelectConfigurator.The
language: "json"prop on Line 4118 appears to be copy-paste residue fromCodeConfigurator.SelectConfiguratordoes not use alanguageprop.🧹 Suggested cleanup
"widget": { "component": "SelectConfigurator", "props": { - "language": "json", "options": [ { "label": "vertical", "value": "vertical" }, { "label": "horizontal", "value": "horizontal" } ] } },
4143-4155: Mixed line endings in defaultValue.The
defaultValueon Line 4146 uses\r\ninitially but ends with just\n, creating inconsistent line endings. This could cause display or parsing inconsistencies.🧹 Suggested fix for consistent line endings
- "defaultValue": "{ \r\n\"barWidth\": 8, \r\n\"barGap\": \"60%\"\n}", + "defaultValue": "{\n \"barWidth\": 8,\n \"barGap\": \"60%\"\n}",
4066-4103: Consider relaxingrequired: truefor optional chart features.Properties like
dataRules,dataZoom,markline,legend,label,tooltip, andlineDataNameare typically optional chart enhancements. Marking all of them asrequired: truemay force users to provide values for features they don't intend to use, reducing configuration flexibility.Consider setting
"required": falsefor genuinely optional properties.
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Show resolved
Hide resolved
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Show resolved
Hide resolved
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Show resolved
Hide resolved
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue
Outdated
Show resolved
Hide resolved
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue
Outdated
Show resolved
Hide resolved
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue
Outdated
Show resolved
Hide resolved
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@designer-demo/public/mock/bundle.json`:
- Around line 4115-4130: Remove the unnecessary "language": "json" property from
the props object of any widget where "component": "SelectConfigurator" (e.g.,
the widget shown with "component": "SelectConfigurator"); locate the props for
SelectConfigurator and delete the language key so only relevant props like
"options" remain, leaving CodeConfigurator usages untouched.
In
`@packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue`:
- Line 22: The component's declared name "ObjectDepthConfigurator" in the
component options should match the file and export name; change the Vue
component option "name" to "NestedPropertyConfigurator" (ensure the object with
name: 'ObjectDepthConfigurator' is updated to name:
'NestedPropertyConfigurator') and then search for any references to
"ObjectDepthConfigurator" (tests, imports, or devtools usage) and update them to
"NestedPropertyConfigurator" so the runtime name, filename, and export remain
consistent.
- Around line 34-46: The computed properties getter (properties) is still
mutating nested objects because the shallow array copy ([...]) doesn't clone
item objects or their nested widget/props; also access to
props.meta.widget.props may throw if props.meta.widget is undefined. Fix by
replacing the forEach mutation with a map that returns brand-new item objects
(spread the item, and also spread item.widget and item.widget.props) and set
widget.props.modelValue on the new object only; guard access to
props.meta.widget before reading props.meta.widget.props (e.g., derive
propsModelValue only when props.meta?.widget exists). Ensure the computed
returns the mapped array of new objects so parent state isn't mutated.
🧹 Nitpick comments (2)
designer-demo/public/mock/bundle.json (2)
4143-4155: Consider using\ninstead of\r\nin defaultValue.The
defaultValueforitemStylecontains Windows-style line endings (\r\n). This could cause inconsistent display or parsing issues across different platforms. Using\nalone is more portable.🧹 Suggested fix
- "defaultValue": "{ \r\n\"barWidth\": 8, \r\n\"barGap\": \"60%\"\n}", + "defaultValue": "{\n \"barWidth\": 8,\n \"barGap\": \"60%\"\n}",
4066-4103: Consider relaxingrequired: truefor optional chart properties.Properties like
dataRules,dataZoom, andmarklineare typically optional chart configurations that users may not need to specify. Marking them all asrequired: truecould create unnecessary friction in the configurator UI or cause validation issues when these optional features aren't needed.Also applies to: 4220-4238
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Outdated
Show resolved
Hide resolved
packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/configurator/src/nested-property-configurator/NestedPropertyConfigurator.vue`:
- Around line 45-50: props.meta.widget is being dereferenced without a guard
causing setup/computed to throw when widget is undefined; update usages in the
setup: initialize bindValue from props.meta?.widget?.props?.modelValue (or
fallback to undefined) and change the properties computed to reference
props.meta?.widget (use optional chaining) where needed so you never access
.props on an undefined widget; specifically update the bindValue assignment and
the computed named properties to use props.meta?.widget?.props?.modelValue and
fallback defaults (e.g., undefined or []) to avoid runtime errors.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
图表组件的option配置面板现在是作为一个整体交给用户配置,体验很不好
Issue Number: N/A
What is the new behavior?
将options里各个属性拆分成单独的配置属性由用户配置
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.