feat: Chart responsible for its own theme #1772

bmingles · 2024-02-05T20:58:18Z

This PR

Move theme colors down to layout.template.layout so that they are used as defaults. Some of the theme colors had to be set in layout.template.data for specific trace types
Chart component now responds to ChartTheme context changes (this should enable chart theme switching, but it depends on some follow up plugins repo work to complete it).

Testing
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:

Renamed ColorUtils.getColorwayFromTheme to normalizeColorway
Removed chartTheme arg from functions in ChartUtils, ChartModelFactory and FigureChartModel in @deephaven/chart

codecov · 2024-02-05T21:33:58Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (f7989b6) 46.07% compared to head (d278f1a) 46.05%.
Report is 5 commits behind head on main.

❗ Current head d278f1a differs from pull request most recent head a75b874. Consider uploading reports for the commit a75b874 to get more accurate results

Files	Patch %	Lines
packages/chart/src/Chart.tsx	0.00%	14 Missing ⚠️
packages/chart/src/MockChartModel.ts	0.00%	4 Missing ⚠️
packages/chart/src/ChartUtils.ts	75.00%	3 Missing ⚠️
...es/dashboard-core-plugins/src/ChartPanelPlugin.tsx	0.00%	2 Missing ⚠️
packages/chart/src/ChartModelFactory.ts	50.00%	1 Missing ⚠️
.../dashboard-core-plugins/src/ChartBuilderPlugin.tsx	0.00%	1 Missing ⚠️
...s/dashboard-core-plugins/src/ChartWidgetPlugin.tsx	0.00%	1 Missing ⚠️

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

Flag	Coverage Δ
unit	`46.05% <27.77%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattrunyon

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

packages/chart/src/Chart.tsx

packages/chart/src/ChartUtils.ts

mattrunyon · 2024-02-07T23:56:25Z

packages/chart/src/ChartUtils.ts

    } else if (plotStyle === dh.plot.SeriesPlotStyle.PIE) {
      seriesData.textinfo = 'label+percent';


Do we want to move the other trace specific things to the template where possible? Could be a separate ticket

Possibly? Since there are some things that will have to stay here, I'm not too worried about it.

packages/chart/src/ChartUtils.ts

mattrunyon · 2024-02-08T00:09:57Z

packages/chart/src/FigureChartModel.ts

    this.layout = {
      grid: {
        rows: figure.rows,
        columns: figure.cols,
        pattern: 'independent',
      },
-      template,
+      template: this.chartUtils.makeDefaultTemplate(theme),


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

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

mattrunyon · 2024-02-08T00:29:11Z

packages/chart/src/plotly/plotly-extends.d.ts

+ * Fix some issues with plotly types.
+ */
+
+declare module 'plotly.js' {


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

It doesn't seem to merge with the existing Plotly types when I do this

deephaven#1728

bmingles · 2024-02-09T18:45:54Z

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

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.

packages/chart/src/ChartUtils.ts

mattrunyon · 2024-02-09T20:26:20Z

packages/chart/src/ChartUtils.ts

-          if (layout[axisLayoutProperty] == null) {
-            layout[axisLayoutProperty] = this.makeLayoutAxis(axisType, theme);
-          }


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 } } } }

Good call. Doing this allowed me to restore most of the full tests I had deleted before which feels better to me.

bmingles mentioned this pull request Feb 6, 2024

feat: Moving theme updating into chart #1761

Closed

bmingles requested a review from mattrunyon February 6, 2024 22:28

mattrunyon requested changes Feb 8, 2024

View reviewed changes

bmingles added 8 commits February 8, 2024 09:20

Added theme colors to chart template

7f5fed2

deephaven#1728

Fixed tests

e22e1cb

deephaven#1728

Unit tests

436993f

deephaven#1728

Updated e2e snapshots

215f629

deephaven#1728

Removed theme colors that were moved to template (deephaven#1728)

f4ebb4e

Adjusted mock chart model for template (deephaven#1728)

0050b44

Fixed issue with mock chart (deephaven#1728)

154411f

Updated e2e snapshots (deephaven#1728)

3aab67e

bmingles force-pushed the 1728-chart-component-theme branch from bc5717f to 3aab67e Compare February 8, 2024 15:21

bmingles added 4 commits February 8, 2024 10:36

Addressing review comments (deephaven#1728)

505bb70

Removed more theme references from utils (deephaven#1728)

36f62d2

Removed charttheme params (deephaven#1728)

4429270

Fixed tests (deephaven#1728)

728e3ab

bmingles requested a review from mattrunyon February 9, 2024 18:39

Added @returns comment (deephaven#1728)

1d0478c

mattrunyon requested changes Feb 9, 2024

View reviewed changes

bmingles added 3 commits February 12, 2024 13:33

Keep axis creation (deephaven#1728)

f67935c

Restored tests (deephaven#1728)

8fc4898

Supress console error in test (deephaven#1728)

456b783

bmingles requested a review from mattrunyon February 12, 2024 20:35

bmingles added 3 commits February 12, 2024 15:48

Fixing axis padding (deephaven#1728)

3b85519

Reverting e2e snapshots (deephaven#1728)

fbeb302

Chart titles centered horizontally based on plot area (deephaven#1728)

caeb348

bmingles force-pushed the 1728-chart-component-theme branch from d278f1a to caeb348 Compare February 12, 2024 22:46

Updated snapshot (deephaven#1728)

a75b874

mattrunyon approved these changes Feb 12, 2024

View reviewed changes

bmingles merged commit fabb055 into deephaven:main Feb 13, 2024
4 checks passed

bmingles deleted the 1728-chart-component-theme branch February 13, 2024 16:43

github-actions bot locked and limited conversation to collaborators Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Chart responsible for its own theme #1772

feat: Chart responsible for its own theme #1772

bmingles commented Feb 5, 2024 •

edited

Loading

codecov bot commented Feb 5, 2024 •

edited

Loading

mattrunyon left a comment

mattrunyon Feb 7, 2024

bmingles Feb 8, 2024

mattrunyon Feb 8, 2024

mattrunyon Feb 8, 2024

mattrunyon Feb 8, 2024

bmingles Feb 8, 2024

bmingles commented Feb 9, 2024

mattrunyon Feb 9, 2024

bmingles Feb 12, 2024

		} else if (plotStyle === dh.plot.SeriesPlotStyle.PIE) {
		seriesData.textinfo = 'label+percent';

feat: Chart responsible for its own theme #1772

feat: Chart responsible for its own theme #1772

Conversation

bmingles commented Feb 5, 2024 • edited Loading

codecov bot commented Feb 5, 2024 • edited Loading

Codecov Report

mattrunyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmingles commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmingles commented Feb 5, 2024 •

edited

Loading

codecov bot commented Feb 5, 2024 •

edited

Loading