-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Improve image graph axis display in email reports #20953
Conversation
…lable, tweak graph margins
@sgiehl I haven't added a UI test for this yet as it would require a report with >1,000 visits and the UI test fixture only has 514 visits in total. Modifying the omnifixture to add another 500 visits which will then slow down all UI test suites doesn't seem like a good idea for a single test. Any other suggestions? |
Could the same issue occur on the x-axis? Could there be a report that has thousands on the x-axis and would be affected by the same problem? Wondering whether we should apply this to all point labels on any axis in a more generic way. |
It's the |
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.
This looks good to me but would still prefer @sgiehl's review as well.
const TOP_GRID_MARGIN_HORIZONTAL_GRAPH = 1; | ||
const RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH = 4; | ||
const TOP_GRID_MARGIN_HORIZONTAL_GRAPH = 10; | ||
const RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH = 20; |
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.
Looking at the changed screenshots it looks like the adjustments were actually only needed for the evolution chart.
The other charts now have additional unused space. Wouldn't it been have possible to only adjust the evolution one?
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.
The other charts are also drawn very close to the edge of the image, for example the horizontal bar chart would likely clip the bar totals for numbers with four digits. I think adding few extra pixels of padding around them reduces the likelihood of other clipping issues and will look better if the images are embedded with other text.
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 guess we would actually need to automatically increase those margins if higher values are meant to be printed.
It might make sense to adjust the UI tests so they use higher values. I think we could achieve that without tracking additional numbers but simply modifying the results. I'll try something an maybe push some changes here to achieve that if easily possible.
@bx80 I've pushed some changes so the imagegraph tests run with higher values. Feel free to check if that looks good to you. Otherwise I think this PR would be read to merge. |
Thanks @sgiehl, the tests should be more robust with higher values 👍 |
Description:
Fixes #17747
When using the 🇫🇷 French language locale, numbers with thousand separators are shown with boxes instead of the expected separator. This is because the number formatter is using a narrow non-breaking space (NNBSP) as the thousand separator which is not supported by all fonts.
This PR makes the following changes to image graphs:
This is the same report using unifont:
And again without unifont:
Review