-
Notifications
You must be signed in to change notification settings - Fork 87
feat!: upgrade Highcharts version to 12.3.0 #10010
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
base: main
Are you sure you want to change the base?
Conversation
Upgrade the Highcharts dependency to 12.3.0.
65ef5f8
to
a8c2d3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
88a8f07
to
2975887
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a bit difficult to review the changes to the base styles, when they’ve changes so much. But the end results look to work as expected 🙂
stroke-width: 1px; | ||
stroke: var(--vaadin-charts-tooltip-border, var(--vaadin-border-color-subtle)); | ||
fill: var(--vaadin-charts-tooltip-background, var(--vaadin-background-color)); | ||
fill-opacity: var(--vaadin-charts-tooltip-background-opacity, 1); | ||
stroke-width: 0; | ||
fill: var(--highcharts-background-color, var(--vaadin-charts-tooltip-background, var(--vaadin-background-color))); | ||
} | ||
|
||
${unsafeCSS(scope)} .highcharts-tooltip-box .highcharts-label-box { | ||
fill: var(--vaadin-charts-tooltip-background, var(--vaadin-background-color)); | ||
fill-opacity: var(--vaadin-charts-tooltip-background-opacity, 1); | ||
stroke: var(--vaadin-charts-tooltip-border, var(--vaadin-border-color-subtle)); | ||
} | ||
|
||
${unsafeCSS(scope)} .highcharts-tooltip-header { | ||
stroke-width: 1px; | ||
stroke: color-mix(in srgb, var(--vaadin-charts-contrast, var(--_label)) 20%, transparent); | ||
fill: var(--highcharts-background-color, var(--vaadin-charts-tooltip-background, var(--vaadin-background-color))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the tooltip border?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is coming from the Highcharts styles:
https://github.com/highcharts/highcharts/blob/d1c0f24d6a15c204a3dfeca42fe7b625e8a83682/css/highcharts.css#L332-L335
} | ||
|
||
${unsafeCSS(scope)} div.highcharts-tooltip { | ||
filter: none; | ||
font-size: 0.8em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the font size nicely smaller in the tooltip. But since the header within it is also using an em
-based font size, that gets even smaller. I’d increase the header font size a bit, perhaps to 0.9em
, if it needs to be smaller than the other text in the tooltip. Maybe add custom properties for controlling the font size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update it to be the same as the current styles. Currently, the font size is inherited from the root with the value: var(--vaadin-charts-font-size, 0.75rem)
. The header currently has a size of 0.85em
instead of the 0.8em
being changed here.
font-family: | ||
-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, 'Apple Color Emoji', 'Segoe UI Emoji', | ||
'Segoe UI Symbol', sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Font family should be inherited.
font-family: | |
-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, 'Apple Color Emoji', 'Segoe UI Emoji', | |
'Segoe UI Symbol', sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
font-family: | ||
-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, 'Apple Color Emoji', 'Segoe UI Emoji', | ||
'Segoe UI Symbol', sans-serif; | ||
font-size: 1rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this. Maybe we should at least have a property for this, or let it inherit. But if we let it inherit, then we should check the other font sizes so that they remain legible even if the font size on the parent is reduced to something like 13 or 14 px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that and attempted to align the font sizes elsewhere to work with our defaults, just as they are in the current version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the colors so different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Legend shows 10 series, but there are only 5 lines visible, and the series titles show they are duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't catch that. Let me see what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turned out the issue was coming from a known change in the behavior of the userOptions
property of the Chart
instance. We needed to add the series manually to the "exporting" one because the userOptions
used to instantiate didn't use to have the information about the series in this case. But it now does, so no need to manually copy over the series into the new chart.
The DOM structure in Highchart V12 is different related to how the drilldown controls are rendered. That will cause test failures in the ITs so we need to updated them. Part of vaadin/web-components#8825 Depends on vaadin/web-components#10010
The DOM structure in Highchart V12 is different related to how the drilldown controls are rendered. That will cause test failures in the ITs so we need to updated them. Part of vaadin/web-components#8825 Depends on vaadin/web-components#10010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the comments about font size.
stroke-width: 1px; | ||
stroke: var(--vaadin-charts-tooltip-border, var(--vaadin-border-color-subtle)); | ||
fill: var(--vaadin-charts-tooltip-background, var(--vaadin-background-color)); | ||
fill-opacity: var(--vaadin-charts-tooltip-background-opacity, 1); | ||
stroke-width: 0; | ||
fill: var(--highcharts-background-color, var(--vaadin-charts-tooltip-background, var(--vaadin-background-color))); | ||
} | ||
|
||
${unsafeCSS(scope)} .highcharts-tooltip-box .highcharts-label-box { | ||
fill: var(--vaadin-charts-tooltip-background, var(--vaadin-background-color)); | ||
fill-opacity: var(--vaadin-charts-tooltip-background-opacity, 1); | ||
stroke: var(--vaadin-charts-tooltip-border, var(--vaadin-border-color-subtle)); | ||
} | ||
|
||
${unsafeCSS(scope)} .highcharts-tooltip-header { | ||
stroke-width: 1px; | ||
stroke: color-mix(in srgb, var(--vaadin-charts-contrast, var(--_label)) 20%, transparent); | ||
fill: var(--highcharts-background-color, var(--vaadin-charts-tooltip-background, var(--vaadin-background-color))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is coming from the Highcharts styles:
https://github.com/highcharts/highcharts/blob/d1c0f24d6a15c204a3dfeca42fe7b625e8a83682/css/highcharts.css#L332-L335
} | ||
|
||
${unsafeCSS(scope)} div.highcharts-tooltip { | ||
filter: none; | ||
font-size: 0.8em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update it to be the same as the current styles. Currently, the font size is inherited from the root with the value: var(--vaadin-charts-font-size, 0.75rem)
. The header currently has a size of 0.85em
instead of the 0.8em
being changed here.
font-family: | ||
-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, 'Apple Color Emoji', 'Segoe UI Emoji', | ||
'Segoe UI Symbol', sans-serif; | ||
font-size: 1rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that and attempted to align the font sizes elsewhere to work with our defaults, just as they are in the current version.
font-family: | ||
-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, 'Apple Color Emoji', 'Segoe UI Emoji', | ||
'Segoe UI Symbol', sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
3ab8029
to
a8e0f64
Compare
|
Description
Upgrade the Highcharts dependency to 12.3.0.
The list of changes introduced in this PR:
--vaadin-charts-*
and--highcharts-*
CSS properties API, with the latter taking precedence, when both are defined.legend-item-click
API to cover the newlegend.events.itemClick
event introduced to replaceseries.events.legendItemClick
.beforeExport
/afterExport
events used by the mixin to copy the styles to the body and do the cleanup afterwards, when the user exports the chart as an image.userOptions
used to be somewhat static, meaning that after the chart was initialized, its value wouldn't update based on the chart's updates. Now, adding/removing items (such as series or points) is reflected in theuserOptions
object.userOptions
object is that properties that accept arrays, such asxAxis
,yAxis
, etc, used to maintain the type of the object it was initially passed in the options, if for instance, an object was passed. That changed so that it always returns an array in theuserOptions
.__hasConfigurationBuffer
method has changed to take into account the case where the property is an array.Part of #8825
Type of change