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(vis_type_vega): Use default categorical color palette directly #3663

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Mar 22, 2023

Description

rather than relying on vega.scheme. Also update vega.scheme name

Before

Screen.Recording.2023-03-27.at.6.37.17.PM.mov

After

(note that the color palette scale is still applied correctly)

Screen.Recording.2023-03-27.at.6.42.25.PM.mov

Possible Breaking Change

The only usage of the elastic custom scheme previously was to set the default value for config.range.category, and that behavior is restored by this PR. However, it was previously possible to explicitly set scheme: "elastic" within a spec - that's no longer possible. I think it's highly unlikely that there's a vega property that accepts a scheme property that doesn't also use the default config.range.category, but it may be possible. I'm also unsure if elastic ever documented any such usage. But my personal judgement is that this is unlikely to break any real user specs.

Issues Resolved

Fixes #3582

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

rather than relying on vega.scheme. Also update vega.scheme name

Fixes opensearch-project#3582

Signed-off-by: Josh Romero <[email protected]>
@joshuarrrr joshuarrrr requested a review from a team as a code owner March 22, 2023 23:33
@joshuarrrr joshuarrrr assigned joshuarrrr and unassigned joshuarrrr Mar 22, 2023
@joshuarrrr joshuarrrr added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Mar 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

Merging #3663 (217d6ad) into main (de06344) will not change coverage.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #3663   +/-   ##
=======================================
  Coverage   66.45%   66.45%           
=======================================
  Files        3208     3208           
  Lines       61593    61593           
  Branches     9502     9502           
=======================================
  Hits        40932    40932           
  Misses      18384    18384           
  Partials     2277     2277           
Flag Coverage Δ
Linux 66.40% <100.00%> (ø)
Windows 66.40% <100.00%> (ø)

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

Impacted Files Coverage Δ
...ins/vis_type_vega/public/data_model/vega_parser.ts 80.67% <100.00%> (ø)
...s/vis_type_vega/public/vega_view/vega_base_view.js 56.09% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@abbyhu2000
Copy link
Member

@joshuarrrr I think this is missing a changelog entry?

@joshuarrrr
Copy link
Member Author

@abbyhu2000 Yeah, it also needs a regression test case.

@joshuarrrr
Copy link
Member Author

I looked more deeply into this, and I can't really figure out what the root cause is. It appears this is a regression caused by bumping vega from 5.22.1 to 5.22.3: #3533

But none of the changes really jump out at me as a possible cause: https://github.com/vega/vega/releases/tag/v5.23.0

Using a reference to the elastic scheme still parses correctly from vega-lite to vega, but fails when executing the vega spec.

Given the above I'm not sure a regression test is particularly helpful in this case. While using the scheme would be nice, the fix here is probably more robust and just as good given our current vega implementation.

@joshuarrrr
Copy link
Member Author

Changelog isn't necessary, as this is a minor re-implementation to fix an unreleased regression.

@joshuarrrr joshuarrrr added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry backport 2.6 labels Mar 27, 2023
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

I think the change seems to be beneficial for improved accessibility and error resolution. If the assessment is correct, the risks should be minimal. I also see tests been added which is good.

@joshuarrrr joshuarrrr merged commit e194bc5 into opensearch-project:main Mar 30, 2023
@joshuarrrr joshuarrrr deleted the fix/vega-color-scheme branch March 30, 2023 18:21
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 30, 2023
…3663)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes #3582

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit e194bc5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 30, 2023
…3663)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes #3582

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit e194bc5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
joshuarrrr pushed a commit that referenced this pull request Apr 4, 2023
…3663) (#3743)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes #3582


(cherry picked from commit e194bc5)

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
vagimeli pushed a commit to vagimeli/OpenSearch-Dashboards that referenced this pull request Apr 4, 2023
…pensearch-project#3663)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes opensearch-project#3582

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: vagimeli <[email protected]>
joshuarrrr pushed a commit that referenced this pull request Apr 5, 2023
…3663) (#3744)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes #3582


(cherry picked from commit e194bc5)

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…pensearch-project#3663)

rather than relying on vega.scheme. Also update vega.scheme name

Fixes opensearch-project#3582

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unrecognized scheme name: elastic when rendering vega charts
6 participants