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

legend item spacing incorrect for empty-name #16398

Closed
ochristensen opened this issue Sep 23, 2021 · 3 comments · Fixed by #16498
Closed

legend item spacing incorrect for empty-name #16398

ochristensen opened this issue Sep 23, 2021 · 3 comments · Fixed by #16498
Assignees

Comments

@ochristensen
Copy link

ochristensen commented Sep 23, 2021

Expected behaviour

legend rows should be spaced the same regardless of name

Actual behaviour

providing a blank name for data leads to the legend row icon being "compressed" against the next row.

Live demo with steps to reproduce

https://jsfiddle.net/zenzizen/maj9hosg/7/ pie chart
https://jsfiddle.net/zenzizen/maj9hosg/14/ scatterplot

Product version

Highcharts & Highstock latest

Affected browser(s)

All presumably; checked Firefox, Brave and Chrome

@highsoft-bot highsoft-bot added this to To do in Development-Flow via automation Sep 23, 2021
@ochristensen ochristensen changed the title pie chart legend item spacing incorrect for empty-name legend item spacing incorrect for empty-name Sep 23, 2021
@raf18seb
Copy link
Contributor

Hi @ochristensen! Thanks for creating the ticket.

The legend item's height is defined by its BBox height which is calculated based on the text height. When there's no text, then bBox.height = 0 and the legend.symbolHeight is taken which, by default, is equal to the text's font size - which is smaller than the BBox text height. That's why the distance between the elements is not constant when the text is missing.

item.itemHeight = bBox.height || legend.symbolHeight;

Let me explain the above on a simple example. When the legend.itemStyle.fontSize equals to e.g. 8px, then:

  • when there is no text, then the bBox.height is 0 and the legend.symbolHeight is 8, so the item.itemHeight is 8
  • when the text exists (series.name is defined), then the bBox.height is 13 - that's why the items spacing is not equal

I'm not sure whether we should treat it as a bug. If you know the font size you are using, then you can simply define the legend.symbolHeight, see demo: https://jsfiddle.net/BlackLabel/3bq8xmyj/

Any thoughts @TorsteinHonsi? Should we treat it as a bug?

@ochristensen
Copy link
Author

thanks for the quick reply Rafał - I figured it was bounding box related given there was no text to measure; background for this is that we allow plotting "missing" values as a distinct category and the legacy backend code that generates our data provides literally an empty string for the name - we can workaround this by inserting a zero-width space as the series name in the front end, but it would be nice if they were consistent heights out-of-the-box regardless.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Sep 30, 2021

@raf18seb It may not be a critical bug, but I do think there's an easy fix, so I suggest we fix it. Since symbolHeight comes directly from font size, it is a mistake using it as a replacement for bBox.height, which better corresponds to the line height than the font size. We already have a computed value for the line height that we can fall back on. So instead of

legend.itemHeight = item.itemHeight = Math.round(
item.legendItemHeight || bBox.height || legend.symbolHeight
);

We can do

        legend.itemHeight = item.itemHeight = Math.round(
            item.legendItemHeight ||
            bBox.height ||
            (legend.fontMetrics && legend.fontMetrics.h)
        );

It may not be pixel perfect in all cases, but it is better.

@highsoft-bot highsoft-bot moved this from To do to Backlog in Development-Flow Sep 30, 2021
@bm64 bm64 self-assigned this Sep 30, 2021
@highsoft-bot highsoft-bot moved this from Backlog to In progress in Development-Flow Sep 30, 2021
@highsoft-bot highsoft-bot moved this from In progress to Review in progress in Development-Flow Oct 15, 2021
@highsoft-bot highsoft-bot moved this from Review in progress to In progress in Development-Flow Oct 15, 2021
@highsoft-bot highsoft-bot moved this from Review in progress to In progress in Development-Flow Oct 15, 2021
bm64 added a commit that referenced this issue Oct 21, 2021
@highsoft-bot highsoft-bot moved this from In progress to Review in progress in Development-Flow Oct 22, 2021
Development-Flow automation moved this from Review in progress to Done Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

5 participants