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

fix(theme): merge partial with empty initial partial #1452

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Nov 1, 2021

Summary

Fix theming override logic to allow initial partial value as empty object ({}).

Before something like the chart below would ignore the dash definition because the first partial is empty.

export const Example = () => {
  return (
    <Chart>
      <Settings
        baseTheme={useBaseTheme()}
        theme={[
          {},
          {
            crosshair: {
              line: {
                dash: [4, 4],
              },
            },
          },
          EUI_CHARTS_THEME_LIGHT.theme,
        ]}
      />
    </Chart>
  );
};

Now the dash is correctly shown.

Screen Recording 2021-11-01 at 06 29 11 PM

Details

The mergePartial logic had two issues...

  1. The merge optional values logic only ran if the first partial was not undefined. Now it checks for any keys on all partial values at the given level and applies mutations to the baseClone as needed, if either the partial has applicable keys or any additionalPartials have applicable keys.
  2. The final mapping over the new baseClone was only looking at the keys of the base and not the baseClone.

Unrelated, deprecates mergeWithDefaultTheme and removes all internal usage.

Issues

Actually fixes #1284

Related to elastic/kibana#116754

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios

@nickofthyme nickofthyme added :styling Styling related issue :all Applies to all chart types :theme labels Nov 1, 2021
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I tested the fix locally and works correctly when merging an empty object as the first partial object.
I think this actually doesn't completely fix the issue in Kibana, and it needs to go hand in hand with: #1453 and #1447

We need to backport those PRs into 38.0.x for Kibana 7.16

@nickofthyme nickofthyme merged commit d1e690a into elastic:master Nov 2, 2021
@nickofthyme nickofthyme deleted the fix-merge-partial branch November 2, 2021 13:36
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Nov 2, 2021
nickofthyme pushed a commit that referenced this pull request Nov 2, 2021
## [38.0.2](v38.0.1...v38.0.2) (2021-11-02)

### Bug Fixes

* **interactions:** line cursor above the chart, band cursor below ([#1453](#1453)) ([#1457](#1457)) ([ca004a6](ca004a6))
* **theme:** merge partial with empty initial partial ([#1452](#1452)) ([#1454](#1454)) ([2eadc71](2eadc71))
* **xy:** show mouse cursors on charts with opaque background ([#1447](#1447)) ([#1455](#1455)) ([b416ed5](b416ed5))
nickofthyme pushed a commit that referenced this pull request Nov 3, 2021
## [38.1.4](v38.1.3...v38.1.4) (2021-11-03)

### Bug Fixes

* **interactions:** line cursor above the chart, band cursor below ([#1453](#1453)) ([d8d7ee0](d8d7ee0))
* **theme:** merge partial with empty initial partial ([#1452](#1452)) ([d1e690a](d1e690a))
@nickofthyme
Copy link
Collaborator Author

🎉 This PR is included in version 38.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types released Issue released publicly :styling Styling related issue :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theme overrides are not applied with missing optional base values
2 participants