feat: Chart responsible for its own theme#1772
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1772 +/- ##
==========================================
- Coverage 46.07% 46.05% -0.03%
==========================================
Files 628 628
Lines 37781 37771 -10
Branches 9516 9512 -4
==========================================
- Hits 17409 17394 -15
- Misses 20317 20323 +6
+ Partials 55 54 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mattrunyon
left a comment
mattrunyon
left a comment
There was a problem hiding this comment.
Looks like the padding on the styleguide snapshots got a little smaller for the charts. I'm guessing it's fine, but wanted to make sure it was a known change
| } else if (plotStyle === dh.plot.SeriesPlotStyle.PIE) { | ||
| seriesData.textinfo = 'label+percent'; |
There was a problem hiding this comment.
Do we want to move the other trace specific things to the template where possible? Could be a separate ticket
There was a problem hiding this comment.
Possibly? Since there are some things that will have to stay here, I'm not too worried about it.
| pattern: 'independent', | ||
| }, | ||
| template, | ||
| template: this.chartUtils.makeDefaultTemplate(theme), |
There was a problem hiding this comment.
It would be ideal if the model didn't need the theme at all, but looks like it does for some dynamic axis stuff still.
You should be able to remove the template at least because it's overridden by Chart anyway
There was a problem hiding this comment.
I did a bit of testing and you can remove theme from the updateFigureAxes call. The only spot where it ends up using it is to set the axis if one doesn't exist, but plotly applies from the template in that case it seems.
Just to test, get a subplotting example from here and then change something in makeLayoutAxis. I made the y axis zerolinecolor red and it properly applied to both series even w/o the assignment in updateLayoutAxes
That then leaves just the error bars in the model which I bet can remove the theme as well since you added it to the default template. I couldn't find a good example on the docs to test this though
| * Fix some issues with plotly types. | ||
| */ | ||
|
|
||
| declare module 'plotly.js' { |
There was a problem hiding this comment.
Can this go in the root @types folder? I'd try putting it there to see if you get the same results. It is meant to serve as a central types override
There was a problem hiding this comment.
It doesn't seem to merge with the existing Plotly types when I do this
bmingles
force-pushed
the
1728-chart-component-theme
branch
from
bc5717f to
3aab67e
Compare
This is a known change. The styleguide was overriding some of the default layout. Now that we have moved to template.layout, the defaults properly stay in place. |
| if (layout[axisLayoutProperty] == null) { | ||
| layout[axisLayoutProperty] = this.makeLayoutAxis(axisType, theme); | ||
| } |
There was a problem hiding this comment.
I wonder if this should set the axis to an empty object instead by default. Then the template part would still be applied by the template, but any other axis specifics (like range I think) should get applied to the axis. The code after calls updateLayoutAxis only if the axis exists. In subplots, the axis wouldn't exist (but it does seem to get created somewhere, I just don't know where)
layout: {
xaxis: ...,
xaxis2: { range: ... },
template: {
layout: {
xaxis: { styleStuff }
}
}
}There was a problem hiding this comment.
Good call. Doing this allowed me to restore most of the full tests I had deleted before which feels better to me.
bmingles
force-pushed
the
1728-chart-component-theme
branch
from
d278f1a to
caeb348
Compare
2 participants
This PR
layout.template.layoutso that they are used as defaults. Some of the theme colors had to be set inlayout.template.datafor specific trace typesTesting
For now, we should just make sure charts behave as they did before. Not all charts will update in response to a theme change, but re-running queries should show the theme change. I have tested this and have not seen any changes to existing behavior introduced by this PR
Once the following plugins repo PRs have merged, the chart should fully respond to theme changes:
deephaven/deephaven-plugins#243
deephaven/deephaven-plugins#251
resolves #1728
BREAKING CHANGE:
ColorUtils.getColorwayFromThemetonormalizeColorwaychartThemearg from functions inChartUtils,ChartModelFactoryandFigureChartModelin @deephaven/chart