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

Long Titles Wrap but Cover the Chart #1878

Open
sosiology opened this issue Nov 14, 2024 · 8 comments · Fixed by #1885 or #1935
Open

Long Titles Wrap but Cover the Chart #1878

sosiology opened this issue Nov 14, 2024 · 8 comments · Fixed by #1885 or #1935
Assignees

Comments

@sosiology
Copy link
Contributor

sosiology commented Nov 14, 2024

To reproduce, see https://int.visualize.admin.ch/en/v/wVfk4PSYRdYk?dataSource=Prod on a smaller screen – the Y axis titles are wrapped, but the chart is not shifted to the bottom, so they are covering the chart.

Image

@sosiology sosiology added bug Something isn't working development and removed bug Something isn't working labels Nov 14, 2024
@sosiology sosiology changed the title Long Titles Wrap but Cover the Chart ✓ Long Titles Wrap but Cover the Chart Nov 25, 2024
@sosiology sosiology changed the title ✓ Long Titles Wrap but Cover the Chart Long Titles Wrap but Cover the Chart Dec 2, 2024
@sosiology sosiology reopened this Dec 2, 2024
@sosiology
Copy link
Contributor Author

sosiology commented Dec 2, 2024

Reopening this @noahonyejese as while testing this issue https://github.com/visualize-admin/visualization-tool-private/issues/110 I noticed the labels cover the charts. I am not sure if this is a regression, or something missed during testing.

Example Charts

Screenshots
Image
Image

Tested on a samsung galaxy s24, using Chrome.

@noahonyejese
Copy link
Contributor

Hi @sosiology , I don't think it's on int yet I tested it on https://test.visualize.admin.ch and it seems to work fine there for screens as small as 375px in width (Iphone SE),

Image

let me know what you think.

@noahonyejese
Copy link
Contributor

@sosiology however I increased the margin a bit

Image

hope this works

@florelina
Copy link

florelina commented Dec 9, 2024

@noahonyejese Thank you.
I've tested the task and there seem to be still some issues:

  • The labels overlap with the visualization and should be moved up

Image

@noahonyejese
Copy link
Contributor

noahonyejese commented Dec 9, 2024

@florelina thanks for the feedback it seems like there won't be really a solution that fits all sizes.

either the smaller screens have too little spacing or the larger screens have too much.

@bprusinowski do you think I should add a condition that makes more space for extra small screen?

Or extra long labels?

@bprusinowski
Copy link
Collaborator

@noahonyejese I think it should be possible to make it work correctly in all cases – it looks that currently in the code we try to compute the new height by multiplying fontSize by the number of overlaps and do it for two axes at the same time, which also could make it not work correctly if we have one really long title and other short one, if I am not mistaken 🤔

I think here we should also take into account things like e.g. line-height, and honestly I think it would be best to simply add a getTextHeight function that would add the text to DOM, constrain the container width, get the correct height and remove the temporary element using this method, eliminating any custom calculations.

What do you think?

@noahonyejese
Copy link
Contributor

@noahonyejese I think it should be possible to make it work correctly in all cases – it looks that currently in the code we try to compute the new height by multiplying fontSize by the number of overlaps and do it for two axes at the same time, which also could make it not work correctly if we have one really long title and other short one, if I am not mistaken 🤔

I think here we should also take into account things like e.g. line-height, and honestly I think it would be best to simply add a getTextHeight function that would add the text to DOM, constrain the container width, get the correct height and remove the temporary element using this method, eliminating any custom calculations.

What do you think?

If we want to add additional calculations for the TextHeight then it might work, but I am not 100% sure if it would cover all cases to be honest. I will try approaching it by creating a getTextHeight , like this we can check if it's better.

@noahonyejese
Copy link
Contributor

@bprusinowski I tried your approach it's slightly better, I think like this it's probably ideal. - thanks for the advice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants