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 colors of visualizations with more than 10 items #7051

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

evamillan
Copy link
Contributor

@evamillan evamillan commented Jun 18, 2024

Description

This PR changes the OUI palette settings used by the visualizations so that they always use the original colors first.

Issues Resolved

Fixes #5422

Screenshot

Before changes:
palette-before

After changes:
palette-after

Testing the changes

A visualization should use the colors in the same order regardless of how many rotations of the color palette it needs.

Changelog

  • fix: Fix colors of the visualizations with more than 10 items

Check List

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

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.45%. Comparing base (170ac61) to head (63b0ff1).

Files Patch % Lines
...ins/charts/public/services/colors/mapped_colors.ts 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7051      +/-   ##
==========================================
- Coverage   67.45%   67.45%   -0.01%     
==========================================
  Files        3448     3448              
  Lines       67957    67960       +3     
  Branches    11055    11057       +2     
==========================================
- Hits        45843    45842       -1     
- Misses      19443    19490      +47     
+ Partials     2671     2628      -43     
Flag Coverage Δ
Linux_1 33.10% <33.33%> (+<0.01%) ⬆️
Linux_2 55.05% <ø> (ø)
Linux_3 45.24% <33.33%> (-0.02%) ⬇️
Linux_4 34.83% <0.00%> (-0.01%) ⬇️
Windows_1 33.12% <33.33%> (+<0.01%) ⬆️
Windows_2 55.00% <ø> (ø)
Windows_3 45.25% <33.33%> (-0.01%) ⬇️
Windows_4 34.83% <0.00%> (-0.01%) ⬇️

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.

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

joshuarrrr
joshuarrrr previously approved these changes Jun 21, 2024
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.

shipit! Thanks @evamillan, this is definitely an improvement. I still think we want to change the behavior for when we need more than 30 colors, as it's a bit weird to stack the dark variations directly against the light variations. But this is the right fix for 10 to 20 colors.

@ananzh
Copy link
Member

ananzh commented Jun 24, 2024

@evamillan seems there is some changelog error to block merging. could you install https://github.com/apps/opensearch-changeset-bot and remove the changelog file? there should be another auto generated changelog file created in changelogs/fragments

ananzh
ananzh previously approved these changes Jun 24, 2024
@evamillan
Copy link
Contributor Author

@evamillan seems there is some changelog error to block merging. could you install https://github.com/apps/opensearch-changeset-bot and remove the changelog file? there should be another auto generated changelog file created in changelogs/fragments

@ananzh I installed the app on my fork and removed the commit that added the file, but it looks like it hasn't been generated. Is there anything else I need to do?

@ananzh
Copy link
Member

ananzh commented Jun 25, 2024

@evamillan loll I just remove the blank line and seems working now.

@ananzh ananzh added the bug Something isn't working label Jun 25, 2024
@ashwin-pc ashwin-pc requested a review from LDrago27 as a code owner June 27, 2024 01:17
@ashwin-pc
Copy link
Member

Rebasing the branch to see if it fixes the CIgroup 3 failure since it seems unrelated to this PR

@ananzh ananzh merged commit e6aa9d6 into opensearch-project:main Jun 27, 2024
65 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 27, 2024
* Fix visualizations colors for more than 10 items
---------

Signed-off-by: Eva Millán <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit e6aa9d6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
LDrago27 pushed a commit that referenced this pull request Jul 25, 2024
* Fix visualizations colors for more than 10 items
---------





(cherry picked from commit e6aa9d6)

Signed-off-by: Eva Millán <[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>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The colors of the visualizations are not displayed correctly
5 participants