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] chart: avoid overlapping values on charts #5028

Closed
wants to merge 1 commit into from

Conversation

hokolomopo
Copy link
Contributor

Description

The option "Show Values" on charts could lead to overlapping texts in the chart, which was quite ugly.

This commit improve the behaviour or the value labels:

  • prevent overlapping value labels on stacked charts
  • the bar chart value labels are now in the middle of the bar
  • the value labels have now the same color as the dataset
  • prevent value labels from going outside of the chart

Code-wise, the commit totally splits the handling of bar and horizontal bar charts. It make the code slightly longer, but avoid having to write a ternary for each computation of x/y position, which ultimately makes the code more readable.

Task: 4207889

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

The option "Show Values" on charts could lead to overlapping texts
in the chart, which was quite ugly.

This commit improve the behaviour or the value labels:
- prevent overlapping value labels on stacked charts
- the bar chart value labels are now in the middle of the bar
- the value labels have now the same color as the dataset
- prevent value labels from going outside of the chart

Code-wise, the commit totally splits the handling of bar and
horizontal bar charts. It make the code slightly longer, but avoid
having to write a ternary for each computation of x/y position, which
ultimately makes the code more readable.

Task: 4207889
@robodoo
Copy link
Collaborator

robodoo commented Sep 27, 2024

Pull request status dashboard

Copy link
Collaborator

@rrahir rrahir left a comment

Choose a reason for hiding this comment

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

👍

It might be worth targeting 18.0, maybe?

},
};
ctx.fillStyle = point.options.backgroundColor;
ctx.strokeStyle = options.background || "#ffffff";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not 100% sure about this one, if you have a very light grey dataset color and no background, the text value becomes unreadable. Maybe having the stroke computed based on point.options.backgroundColor would be better to ensure the text is visible?
fyi, there are 2 datasets in the picture :p
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the text stroke be the same color as the background is ususally prettier. Here for example if we do a white or black text bacgrkound it's not as pretty :
image
image

But indeed there are cases where it'll be ugly. I was thinking that if the user pick shitty colors that's his problem, GSheet does the same:
image

I'll ask Francois

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@hokolomopo
Copy link
Contributor Author

👍

It might be worth targeting 18.0, maybe?

Yeah I wasn't sure what to target. I'd say either 17.4 where we introduced the feature or master

@hokolomopo
Copy link
Contributor Author

backported to 16.4 #5099

@hokolomopo hokolomopo closed this Oct 16, 2024
@LucasLefevre LucasLefevre deleted the master-chart-show-values-adrm branch October 22, 2024 06:48
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