Skip to content
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

[IMP] charts: allow to reorder data series #5027

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anhe-odoo
Copy link
Contributor

Task Description

This task aims to add the possibility to reorder the ranges in the selection input, allowing then the user to change the order of the data series.

Related Task

  • Task: 3994495

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Sep 27, 2024

Pull request status dashboard

Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

👋

Kinda bugged unfortunaltly :x

src/components/icons/icons.xml Outdated Show resolved Hide resolved
src/components/icons/icons.xml Outdated Show resolved Hide resolved
src/components/icons/icons.xml Show resolved Hide resolved
src/components/selection_input/selection_input.ts Outdated Show resolved Hide resolved
src/components/selection_input/selection_input.xml Outdated Show resolved Hide resolved
src/components/selection_input/selection_input.ts Outdated Show resolved Hide resolved
src/components/selection_input/selection_input.ts Outdated Show resolved Hide resolved
@anhe-odoo anhe-odoo force-pushed the master-selection_input-allow_reordering-anhe branch from 8e8145a to 81c6192 Compare October 8, 2024 07:44
Task Description

This task aims to add the possibility to reorder the ranges in the
selection input, allowing then the user to change the order of the
data series.

Related Task

Task: 3994495
@anhe-odoo anhe-odoo force-pushed the master-selection_input-allow_reordering-anhe branch from 81c6192 to f4da0f1 Compare October 21, 2024 08:58
Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks, otherwise LGTM 🥳

@@ -119,6 +119,13 @@ export class GenericChartConfigPanel extends Component<Props, SpreadsheetChildEn
});
}

onDataSeriesReordered(indexes: number[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we have the SelectionInput return an array of index for this callback rather that the final array of ranges ?

From ouside the component, it'd be easire to reaceive an array of range. We could then use onDataSeriesRangesChanged for both props onSelectionReordered and onSelectionChanged. Or is there a reason it doesn't work ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it that way because we have to reorder the ranges and the corresponding properties, and we have to differentiate when the user manually change the ranges and when he reordered it

tests/figures/chart/charts_component.test.ts Show resolved Hide resolved
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.

3 participants