Skip to content

feat: migrate chart-edit config to Formly dynamic forms#1654

Closed
gugu wants to merge 2 commits intomainfrom
formly
Closed

feat: migrate chart-edit config to Formly dynamic forms#1654
gugu wants to merge 2 commits intomainfrom
formly

Conversation

@gugu
Copy link
Contributor

@gugu gugu commented Mar 9, 2026

Replace manual chart configuration template with @ngx-formly/core driven forms. Adds custom field types (repeat-section, color-picker, color-palette, checkbox, expansion-panel wrapper) and a JSON-schema-based field builder for chart options.

Replace manual chart configuration template with @ngx-formly/core driven
forms. Adds custom field types (repeat-section, color-picker, color-palette,
checkbox, expansion-panel wrapper) and a JSON-schema-based field builder for
chart options.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 14:03
@gugu gugu requested a review from lyubov-voloshko March 9, 2026 14:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the chart configuration UI in the chart editor from a manually maintained Angular template to a schema-driven, @ngx-formly/core-based dynamic form system, with custom field types and wrappers.

Changes:

  • Added @ngx-formly/core and @ngx-formly/material dependencies and wired global Formly configuration into app bootstrap.
  • Introduced a JSON-schema-driven Formly field builder for chart options plus custom Formly types (repeat section, color picker/palette, checkbox) and an expansion-panel wrapper.
  • Refactored ChartEditComponent to render configuration via <formly-form> and map the Formly model into widget_options.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
frontend/yarn.lock Locks added Formly dependencies.
frontend/package.json Adds Formly packages to dependencies.
frontend/src/main.ts Registers Formly modules/config during bootstrap via FORMLY_IMPORTS.
frontend/src/app/formly/formly-config.ts Central Formly registration for custom types/wrappers and validation messages.
frontend/src/app/formly/chart-options.schema.ts Defines the chart options JSON schema and grouped unit options.
frontend/src/app/formly/chart-options-fields.ts Builds Formly field configs from schema + UI overrides for the chart editor.
frontend/src/app/formly/repeat-section.type.ts Custom array/repeat UI for series lists.
frontend/src/app/formly/color-picker.type.ts Custom color picker field type for series color.
frontend/src/app/formly/color-palette.type.ts Custom palette editor type (add/remove/defaults).
frontend/src/app/formly/palette-color-input.type.ts Palette color input type with rgba→hex display conversion.
frontend/src/app/formly/checkbox.type.ts Custom checkbox rendering for boolean fields.
frontend/src/app/formly/expansion-panel.wrapper.ts Wrapper to group option sections into expansion panels.
frontend/src/app/components/charts/chart-edit/chart-edit.component.ts Switches chart config state to a Formly model and maps it to widget options.
frontend/src/app/components/charts/chart-edit/chart-edit.component.html Replaces manual chart config template with <formly-form>.
frontend/src/app/components/charts/chart-edit/chart-edit.component.css Updates styling hooks to target Formly-rendered DOM via ::ng-deep.
frontend/src/app/components/charts/chart-edit/chart-edit.component.spec.ts Updates unit tests to reflect the new Formly model-based approach.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +80 to +86
loadDefaults(): void {
const defaults = [
'rgba(99, 102, 241, 0.7)',
'rgba(16, 185, 129, 0.7)',
'rgba(245, 158, 11, 0.7)',
'rgba(239, 68, 68, 0.7)',
'rgba(139, 92, 246, 0.7)',
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadDefaults() hard-codes a palette that duplicates (and differs from) DEFAULT_COLOR_PALETTE in chart-config.helper.ts. This makes it easy for the default palette used by charts and the palette loaded in the editor to drift. Consider importing and reusing the shared DEFAULT_COLOR_PALETTE constant (or centralizing defaults in one place).

Copilot uses AI. Check for mistakes.
Comment on lines +241 to +242
hide: (field) => isPieType(field.parent?.parent?.parent?.model),
},
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPieType expects a ChartOptionsModel, but this expression can pass undefined when the parent chain isn't available yet (field.parent?.parent?.parent?.model). That will throw at runtime when isPieType reads model.chartType. Make isPieType accept ChartOptionsModel | undefined (and return false for undefined) or guard before calling it in the expression.

Copilot uses AI. Check for mistakes.
Comment on lines +248 to +250
const root = field.parent?.parent?.parent?.model;
return isPieType(root) || root?.chartType === 'bar';
},
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hide expression calls isPieType(root) even when root can be undefined (const root = field.parent?.parent?.parent?.model). That will cause a runtime error in the form config. Guard root before calling isPieType, or change isPieType to safely handle undefined/partial models.

Copilot uses AI. Check for mistakes.
},
{
fieldGroupClassName: 'series-options',
expressions: { hide: (field) => isPieType(field.parent?.parent?.model) },
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPieType(field.parent?.parent?.model) can pass undefined during initialization, which will throw when isPieType accesses chartType. Add a null/undefined guard (or make isPieType tolerant of undefined) before calling it here.

Copilot uses AI. Check for mistakes.
Comment on lines 117 to +121
protected hasChartData = computed(() => {
const hasLabel = !!this.labelColumn();
if (this.seriesMode() === 'column') {
return this.testResults().length > 0 && hasLabel && !!this.seriesColumn() && !!this.seriesValueColumn();
const model = this.chartModel;
const hasLabel = !!model.labelColumn;
if (model.seriesMode === 'column') {
return this.testResults().length > 0 && hasLabel && !!model.seriesColumn && !!model.seriesValueColumn;
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasChartData depends on this.chartModel, but chartModel is a plain field (not a signal) so this computed will not recompute when the Formly model changes. This can leave the preview state stale after users change chart settings. Either make chartModel a signal, or add this._modelVersion() (or another signal) as a dependency inside this computed.

Copilot uses AI. Check for mistakes.
Comment on lines +192 to +193
protected chartType = computed(() => this.chartModel.chartType as ChartType);

Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chartType is a computed derived from this.chartModel, but chartModel is not reactive. Since this computed has no signal dependencies, it will never update after the initial evaluation, yet it's used in the template for <app-chart-preview [chartType]="chartType()">. Tie it to _modelVersion() or make chartModel a signal so chart type changes propagate.

Copilot uses AI. Check for mistakes.
Comment on lines 25 to 26
import { DEFAULT_COLOR_PALETTE } from 'src/app/lib/chart-config.helper';
import {
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT_COLOR_PALETTE is imported but no longer referenced in this component. If linting is enabled for unused imports, this will fail CI; consider removing the import or reusing it where appropriate.

Copilot uses AI. Check for mistakes.
- Fix chartType/hasChartData computeds not reacting to model changes
  (missing _modelVersion signal dependency)
- Fix wrong parent chain depth in series field hide expressions
  (replaced fragile parent traversal with getRootModel helper)
- Deep-clone seriesList/colorPalette in onModelChange to avoid
  shared references with Formly internals
- Remove redundant testResults() dependency from currentWidgetOptions
- Consolidate double _modelVersion bumps in testQuery
- Memoize columnOptions to avoid repeated allocations per expression cycle
- Use DEFAULT_COLOR_PALETTE from shared helper instead of duplicate
- Remove unused imports (MatExpansionModule, MatSelectModule,
  DEFAULT_COLOR_PALETTE)
- Use stable track by f.id instead of $index in repeat types

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gugu gugu closed this Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants