-
Notifications
You must be signed in to change notification settings - Fork 60
[IMP] chart: add zoom interactivity #6046
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
base: master
Are you sure you want to change the base?
Conversation
9a3134f
to
00a838f
Compare
00a838f
to
7b6524f
Compare
761856b
to
2ebab79
Compare
2ebab79
to
0664fd5
Compare
src/components/side_panel/chart/scatter_chart/scatter_chart_config_panel.xml
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chartjs.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chartjs.ts
Outdated
Show resolved
Hide resolved
style="margin-right:2.5rem!important"> | ||
<div class="d-flex"> | ||
<div class="o-figure-menu-item mx-1" t-on-click="resetZoom"> | ||
<t t-call="o-spreadsheet-Icon.HOME"/> |
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.
I would display this icon only if the chart is zoomed
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.
I totally agree, but (at the moment) the "is zoomed" state is something that is only known in the ZoomableChartJs component, and there is no ways to get the information in the figure as it's not stored in any definition or runtime :/
I can add something to get the information, but I'm not sure it will be clean :/
src/components/side_panel/chart/bar_chart/bar_chart_config_panel.xml
Outdated
Show resolved
Hide resolved
@@ -50,7 +51,7 @@ chartSidePanelComponentRegistry | |||
design: ChartWithAxisDesignPanel, | |||
}) | |||
.add("combo", { | |||
configuration: GenericChartConfigPanel, | |||
configuration: ComboConfigPanel, |
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.
would it be worth introducing a GenericZoomableChartConfigPanel
?
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.
Not sure, as the only "new" config panel is for the ComboBox, so it will be introducing this one only for one chart type. What do you think ?
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chartjs.ts
Outdated
Show resolved
Hide resolved
57d68ed
to
6810f8e
Compare
b872295
to
1a4da18
Compare
922fea0
to
347b6f2
Compare
ec29382
to
918ec52
Compare
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.
Heyo 👋
didn't read the test 🙃
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
for (const axisId of __ZOOMABLE_AXIS_IDS__) { | ||
if (limits[axisId]) { |
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.
for (const axisId of __ZOOMABLE_AXIS_IDS__) { | |
if (limits[axisId]) { | |
for (const axisId in limits) { |
no ?
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.
Updated with an else clause no that limits can be undefined too :)
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
const zoom = { | ||
...definition.zoom, | ||
enabled, | ||
}; | ||
if (enabled) { | ||
zoom.sliceable = true; | ||
} |
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.
ther's no need for both zoom.sliceable and zoom.enable no ? And if there's a need for both, why is there a single checkbox to handle two booleans ?
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.
Because the idea of the zoom.enable if that the dashboard maker decide that this chart should be zoomable or not in the dashboard. The zoom.sliceable is there when the dashboard/spreadsheet decide to show the slicer. So there is one checkbox to handle zoom.enable, and one button on the chart to show the slicer
isInvalid="isLabelInvalid" | ||
onSelectionChanged.bind="onLabelRangeChanged" | ||
onSelectionConfirmed.bind="onLabelRangeConfirmed" | ||
options="this.getLabelRangeOptions()" |
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.
shouldn't the checkbox be with the other label range options ? Or in the design panel ?
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.
TBC but this has been discussed with FRGI
3f0bcf0
to
80afaa2
Compare
80afaa2
to
aef12da
Compare
aef12da
to
972bc19
Compare
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.
partial review because I have a question 😉
I don't really see why we couldn't move all the configuration changes (getDetailChartConfiguration, etc.) and the slicer config to the plugins (within and along side getChartRuntime
), with a few UI commands to update the limits.
Is there something based on something chartJs computes that we couldn't give to the plugins ?
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chartjs.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Outdated
Show resolved
Hide resolved
src/components/figures/chart/chartJs/zoomable_chart/zoomable_chart_store.ts
Show resolved
Hide resolved
9006818
to
a728b19
Compare
Task Description This PR aims to add the possibility to zoom/pan in a chart, within a dedicated component, in the form of a Master/Detail chart. The zoom interactivity can be enabled in the design panel on line, scatter, bar and combo chart. Related Task Task: 4609219
a728b19
to
1ff1184
Compare
Task Description
This PR aims to add the possibility to zoom/pan in a chart. Two
ways of manipulating the chart have been add :
The zoom interactivity can be enabled by the config panel on line,
scatter, bar and combo chart. The dedicated component should be
activated with the "zoom" icon on the chart, next to the menu icon,
which is only visible if the "zoom" option is activated in the
config menu.
Related Task