-
Notifications
You must be signed in to change notification settings - Fork 31
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] chart: improve pie chart #3545
Conversation
949e009
to
7029aaf
Compare
7029aaf
to
48e60fd
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.
ChartJS sounds like black magic 🧙
Do you think it would simplify things if instead of highlighting the hovered pie part by making every other part transparent, you would use "easier" chartJs features to make the hovered items darker/add a border/make in bigger (this ) ?
You'd still need to do magic when hovering the legend tho...
src/components/side_panel/chart/building_blocks/label_range/label_range.ts
Outdated
Show resolved
Hide resolved
33365e8
to
65708b2
Compare
65708b2
to
bb60e10
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.
would it be possible to split the 3 points in 3 different commits ?
It's easier to review now and for the future as well in case we need to dig in the history
bb60e10
to
5cf4ddd
Compare
5cf4ddd
to
bc812ad
Compare
src/components/side_panel/chart/building_blocks/label_range/label_range.ts
Outdated
Show resolved
Hide resolved
@@ -197,6 +204,56 @@ export class PieChart extends AbstractChart { | |||
const definition = this.getDefinitionWithSpecificDataSets(dataSets, labelRange); | |||
return new PieChart(definition, this.sheetId, this.getters); | |||
} | |||
|
|||
highlightItem(index: number, dataSets) { |
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.
can dataset be typed?
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.
Some comments/suggestions on the first commit :)
The second commit looks like a lot of code for a limited value. I also don't really understand why the datasets should be highlighted for pie charts but not other charts.
I think I'd cancel this part (see message in the task chatter)
src/components/side_panel/chart/building_blocks/label_range/label_range.ts
Outdated
Show resolved
Hide resolved
src/components/side_panel/chart/line_chart/line_chart_config_panel.ts
Outdated
Show resolved
Hide resolved
src/components/side_panel/chart/scatter_chart/scatter_chart_config_panel.ts
Outdated
Show resolved
Hide resolved
7152909
to
95f249b
Compare
95f249b
to
a1af3af
Compare
const zone = toUnboundedZone(this.dataSeriesRanges[0]); | ||
disabled = | ||
zone.left !== undefined && | ||
zone.right !== undefined && | ||
zone.left === zone.right && | ||
this.labelRange === this.dataSeriesRanges[0]; |
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.
actually, I'm not sure why we should prevent the user to uncheck the aggregate.
Task Description This commit implements the ability for the user to plot a pie chart only based on the count of strings in the datasets. The bar and scatter chart has also been updated to take into account the aggregated options when changing chart type, so that the string occurence is kept when switching chart type from the side pannel. Related Task: Task: 3370683
a1af3af
to
5bc91df
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.
robodoo r+
Task Description This commit implements the ability for the user to plot a pie chart only based on the count of strings in the datasets. The bar and scatter chart has also been updated to take into account the aggregated options when changing chart type, so that the string occurence is kept when switching chart type from the side pannel. Related Task: closes #3545 Task: 3370683 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Task Description
The aims of this task is to improve the user experience when creating a pie chart :
Related Task:
review checklist