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.

Sign up for GitHub

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

Jump to bottom

[Manual][Backport 2.x][Vis colors] Update legacy mapped colors in charts plugin to use ouiPaletteColorBlind() #4451

Merged
merged 2 commits into from
Jul 26, 2023
Merged

[Manual][Backport 2.x][Vis colors] Update legacy mapped colors in charts plugin to use ouiPaletteColorBlind() #4451

merged 2 commits into from
Jul 26, 2023

Conversation

manasvinibs
Copy link
Member

@manasvinibs manasvinibs commented Jun 30, 2023 •
edited by ananzh
Loading

Backport PR:

#4398

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

@codecov
Copy link

codecov bot commented Jun 30, 2023 •
edited
Loading

Codecov Report

Merging #4451 (174f4dc) into 2.x (5b8b23a) will increase coverage by 0.19%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##              2.x    #4451      +/-   ##
==========================================
+ Coverage   66.17%   66.36%   +0.19%     
==========================================
  Files        3315     3245      -70     
  Lines       63890    62427    -1463     
  Branches     9961     9668     -293     
==========================================
- Hits        42277    41429     -848     
+ Misses      19171    18648     -523     
+ Partials     2442     2350      -92
Flag Coverage Δ
Linux_1 ?
Linux_2 ?
Linux_3 ?
Linux_4 ?
Windows 66.36% <100.00%> (?)
Windows_1 ?
Windows_2 ?
Windows_3 ?
Windows_4 ?

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

Impacted Files Coverage Δ
...ins/charts/public/services/colors/mapped_colors.ts 92.59% <100.00%> (+0.28%) ⬆️

... and 165 files with indirect coverage changes

ashwin-pc
ashwin-pc previously approved these changes Jun 30, 2023
View reviewed changes
ashwin-pc
ashwin-pc reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

@manasvinibs can you resolve the conflicts in the PR?

ananzh
ananzh previously approved these changes Jul 1, 2023
View reviewed changes
@AMoo-Miki
Copy link
Collaborator

I guess the test failure is in src/plugins/charts/public/services/colors/colors.test.ts which is one of the conflicting files.

@manasvinibs
Copy link
Member Author

This requires more manual testing and might need iterative changes based on feedback from product. Not backporting this PR to 2.x till 2.9 is out as this needs to go in 2.10.

@manasvinibs manasvinibs closed this Jul 5, 2023
@manasvinibs
Copy link
Member Author

Re-opening the PR as 2.9 branch is already cut and this will go as part of 2.10.

@manasvinibs manasvinibs reopened this Jul 12, 2023
@manasvinibs manasvinibs requested a review from BSFishy as a code owner July 12, 2023 18:36
@joshuarrrr
Copy link
Member

@manasvinibs Looks like there are some conflicts to resolve here.

@manasvinibs
[Vis colors] Update legacy mapped colors in charts plugin to use ouiP…
c26a7bb 
…aletteColorBlind() (opensearch-project#4398)

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>
(cherry picked from commit 5e8a4fd)
@manasvinibs manasvinibs dismissed stale reviews from ananzh and ashwin-pc via 49c8924 July 18, 2023 00:04
@manasvinibs manasvinibs force-pushed the backport/backport-4398-to-2.x branch from 174f4dc to 49c8924 Compare July 18, 2023 00:04
@manasvinibs manasvinibs force-pushed the backport/backport-4398-to-2.x branch from 49c8924 to 6622ac2 Compare July 18, 2023 00:11
@manasvinibs @ashwin-pc
[Vis colors] Update legacy mapped colors in charts plugin to use ouiP…
ca65d72 
…aletteColorBlind() (opensearch-project#4398)

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>
(cherry picked from commit 5e8a4fd)
Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
@manasvinibs manasvinibs force-pushed the backport/backport-4398-to-2.x branch from 6622ac2 to ca65d72 Compare July 18, 2023 00:16
@manasvinibs manasvinibs requested review from ashwin-pc and ananzh July 18, 2023 00:17
@manasvinibs manasvinibs added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Jul 18, 2023
@joshuarrrr joshuarrrr self-assigned this Jul 19, 2023
joshuarrrr
joshuarrrr requested changes Jul 24, 2023
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@manasvinibs I want to hold off on merging this backport for a bit until we figure out the color mapping.

No changes needed to this PR, it's a question of deciding with UX.

@manasvinibs
Copy link
Member Author

@manasvinibs I want to hold off on merging this backport for a bit until we figure out the color mapping.

No changes needed to this PR, it's a question of deciding with UX.

Sounds good!

@joshuarrrr
Copy link
Member

I think we should go ahead and merge this and then iterate on top

@joshuarrrr joshuarrrr merged commit a85f798 into opensearch-project:2.x Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BSFishy BSFishy BSFishy approved these changes

@joshuarrrr joshuarrrr joshuarrrr approved these changes

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@seanneumann seanneumann Awaiting requested review from seanneumann

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@abbyhu2000 abbyhu2000 Awaiting requested review from abbyhu2000 abbyhu2000 is a code owner

@zengyan-amazon zengyan-amazon Awaiting requested review from zengyan-amazon zengyan-amazon is a code owner

@kristenTian kristenTian Awaiting requested review from kristenTian

@zhongnansu zhongnansu Awaiting requested review from zhongnansu zhongnansu is a code owner

@ZilongX ZilongX Awaiting requested review from ZilongX ZilongX is a code owner

@Flyingliuhub Flyingliuhub Awaiting requested review from Flyingliuhub Flyingliuhub is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@ananzh ananzh Awaiting requested review from ananzh ananzh is a code owner

Assignees

@joshuarrrr joshuarrrr

Labels
Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@manasvinibs @AMoo-Miki @joshuarrrr @BSFishy @ashwin-pc @ananzh