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/improve-fix-toogling-inverted-packedbubble #20434

Merged

Conversation

hubertkozik
Copy link
Member

@hubertkozik hubertkozik commented Jan 12, 2024

Improved series.invertible logic due to failed tests in #20328.

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Jan 12, 2024

File size comparison

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

master candidate difference
highcharts.js 96.2 kB
271.9 kB
96.1 kB
271.8 kB
-25 B
-57 B
highstock.js 128.8 kB
373.3 kB
128.8 kB
373.2 kB
-23 B
-57 B
highmaps.js 121.6 kB
349.9 kB
121.6 kB
349.8 kB
-23 B
-57 B
highcharts-gantt.js 131.1 kB
378.1 kB
131.1 kB
378.0 kB
-22 B
-57 B

@hubertkozik hubertkozik changed the title bugfix/update-fix-toogling-inverted-packedbubble bugfix/improve-fix-toogling-inverted-packedbubble Jan 12, 2024
@highsoft-bot
Copy link
Collaborator

Visual test results - No difference found

Comment on lines 4303 to 4308
series.invertible = seriesTypes[newType].prototype.invertible;

if (!series.invertible) {
preserve.splice(preserve.indexOf('invertible'), 1);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite get this. Now that you correctly moved the invertible property to the prototype instead of the instance, I expected this to sort itself out. I expected that we could remove invertible from the preserve array, and every series regardless of update, remove or destroy would have the correct value of invertible from its prototype....

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this code for this kind of situation: https://jsfiddle.net/BlackLabel/ya9sjxLv/ Init load bar chart (inverted) and then update the series to the packedbubble and then update chart to inverted. If you remove those lines, then the invertible in preserve array will take over and the logic will be broken: https://jsfiddle.net/BlackLabel/06zjL4wo/

Copy link
Member Author

Choose a reason for hiding this comment

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

And if we remove the invertible from preserve array, then the unit-tests/series-xrange/xrange and unit-tests/chart/chart-update-chart-1 are failing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's because invertible was set on the instance of the Series class, while all others had it in the prototype. Moved it to the prototype and now it seems to pass without the hack: c67581a

@TorsteinHonsi TorsteinHonsi merged commit b60bc50 into master Jan 16, 2024
11 checks passed
@highsoft-bot highsoft-bot added this to the Next milestone Jan 16, 2024
@TorsteinHonsi TorsteinHonsi deleted the bugfix/update-fix-toogling-inverted-packedbubble branch January 16, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants