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

bugfix/20264-toogling-inverted-packedbubble #20328

Merged
merged 4 commits into from Jan 12, 2024

Conversation

hubertkozik
Copy link
Member

@hubertkozik hubertkozik commented Dec 19, 2023

Fixed #20264, the packed bubble series was not rendered correctly in an inverted chart.


@hubertkozik hubertkozik added Product: Highcharts Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog. labels Dec 19, 2023
@hubertkozik hubertkozik self-assigned this Dec 19, 2023
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Dec 19, 2023

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
highcharts.js 96.0 kB
272.1 kB
96.1 kB
272.3 kB
61 B
180 B
highstock.js 128.7 kB
373.2 kB
128.7 kB
373.4 kB
58 B
180 B
highmaps.js 121.4 kB
350.0 kB
121.5 kB
350.2 kB
61 B
180 B
highcharts-gantt.js 130.8 kB
377.9 kB
130.9 kB
378.1 kB
59 B
180 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Dec 19, 2023

Visual test results - No difference found

Copy link

@KacperMadej KacperMadej left a comment

Choose a reason for hiding this comment

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

👍

@hubertkozik hubertkozik marked this pull request as ready for review December 21, 2023 09:15
Comment on lines 2397 to 2403
// If chart is set to inverted and there are no axes, clip box shouldn't
// be inverted (#20264)
if (chart.inverted && !xAxis && !yAxis) {
[seriesBox.width, seriesBox.height] =
[seriesBox.height, seriesBox.width];
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The OP demo works fine even without this block, but the unit tests fail. I don't quite understand why this is necessary - why do we need to flip the axes when we set inverted correctly in Series.update()? Is it because the series clipBox extends the chart.clipBox? And if so, shouldn't we make sure chart.clipBox is correct after running chart.propFromSeries in series.update()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

PS: The chart.clipBox seems to suffer from the same issue here, and continues to do so with the current state of this PR: https://jsfiddle.net/highcharts/oydrk26s/

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I added this code to fix the issue from the demo that you attached: https://jsfiddle.net/BlackLabel/7qLo2x46/ and added a unit test specially for that.

I have tested it on this demo (packedbubble without axes): https://jsfiddle.net/BlackLabel/uv1943nc/

I am not sure about firing something like this after propFromSeries, I tried, but always gave me bad results, that's why I decided to change only series.clipBox to not ruin other logic. Besides that, currently, it works like this for any series with axes, chart.clipBox is calculated and then series.clipBox is overwritten in series.getClipBox with axis.len value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I understand now. The chart.clipBox is actually correct after the update (it should be inverted). What you made sure is that the series that do not support inversion (packedbubble, pie etc), are not clipped to an inverted clip box. I'll do some more checking then come back. For example, some series like sankey or tree graphs support inversion but have no axes. Maybe we need a series flag like invertible if we can't find some existing property to check for.

@TorsteinHonsi TorsteinHonsi merged commit 45f87a6 into master Jan 12, 2024
10 of 11 checks passed
@highsoft-bot highsoft-bot added this to the Next milestone Jan 12, 2024
@TorsteinHonsi TorsteinHonsi deleted the bugfix/20264-toogling-inverted-packedbubble branch January 12, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog. Product: Highcharts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle Chart Type Not Rendering Correctly In All Cases
4 participants