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

Fix cut off axis labels #208

Merged
merged 4 commits into from
Feb 18, 2021
Merged

Fix cut off axis labels #208

merged 4 commits into from
Feb 18, 2021

Conversation

manuelroth
Copy link
Contributor

@manuelroth manuelroth commented Feb 17, 2021

This PR fixes a bug in the text measure logic. The chart tool uses the node-canvas module to measure the width of a text label on server-side. This width is used to determine if a label should be hidden or how much space should be made available for the label.

The bug was that font string that is passed to the canvas context looked like this before context.font = "100 11 nzz-sans-serif". The font string has to follow the same logic as a regular css font string. So it was missing the px for the font-size value. This PR fixes that to context.font = "100 11px nzz-sans-serif". The canvas module couldn't parse this string before and used the default font string which is 10 sans-serif. Which uses a sans-serif font which is installed on the system.

This doesn't fully solve the issue of cut off axis labels though. Therefore this PR adds a padding of 4px to the left of the graphic. This fixes all reported cases of cut off axis labels on web and print versions.

Before: https://q.st.nzz.ch/item/340c84efacc301e257fa36acb658f192

Screenshot 2021-02-17 at 21 07 21

After: https://q.st-test.nzz.ch/item/e9046b127bd99afc9cd208b94d18f29c

Screenshot 2021-02-17 at 21 08 35

More item where this problem can be reproduced:
Before: https://q.st-staging.nzz.ch/item/e9046b127bd99afc9cd208b94d1927ce
After: https://q.st-test.nzz.ch/item/e9046b127bd99afc9cd208b94d1925e6
Before: https://q.st-staging.nzz.ch/item/e9046b127bd99afc9cd208b94d1922bf
After: https://q.st-test.nzz.ch/item/e9046b127bd99afc9cd208b94d191357
Before: https://q.st-staging.nzz.ch/item/e9046b127bd99afc9cd208b94d1900bb
After: https://q.st-test.nzz.ch/item/e9046b127bd99afc9cd208b94d190e9d

Basecamp ToDo: https://3.basecamp.com/3500782/buckets/1333707/todos/2273519802

@manuelroth manuelroth changed the title Fix text measure Fix cut off axis labels Feb 17, 2021
Copy link
Contributor

@KiaraEdTech KiaraEdTech left a comment

Choose a reason for hiding this comment

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

Great catch! Seems like it was a nifty bug. The results are convincingly solve the issue, i played around a bit and did not notice any problem.

PS: I don't quite understand the extra line in docker file.

@manuelroth
Copy link
Contributor Author

manuelroth commented Feb 18, 2021

This PR used to include RUN npm install canvas --build-from-source in the Dockerfile. This was included in an attempt to find the cause of the wrong text width calculation. I will explain what I have found out so far:

  • I noticed that the text width differed when running the tool locally on macOS versus on the docker environment on linux.
  • I found an issue on the node-canvas repository which explains that fonts rendering can differ a lot between different operating systems.
  • The maintainer of node-canvas suggested in another issue that building the canvas module from source could be a solution when loading custom fonts (which we do). Thats why I decided to add it.
  • I found out that this did not solve the cut off label issue. Therefore I decided to add a padding to the chart and I removed the build-from-source command again from the Dockerfile.

@manuelroth manuelroth merged commit 841df0e into dev Feb 18, 2021
@manuelroth manuelroth deleted the fix-text-measure branch February 18, 2021 10:34
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.

2 participants