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

feat: improve color consistency (save all labels) #19038

Merged
merged 51 commits into from Mar 21, 2022

Conversation

stephenLYZ
Copy link
Contributor

@stephenLYZ stephenLYZ commented Mar 6, 2022

SUMMARY

The difference between this PR and this one is that all label colors of the dashboard are saved to handle the native filter scenario.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

2022-03-06.11.02.05.mov

after

2022-03-06.11.00.34.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@jinghua-qa
Copy link
Member

@jinghua-qa jinghua-qa commented Mar 11, 2022

/testenv up

@github-actions
Copy link

@github-actions github-actions bot commented Mar 11, 2022

@jinghua-qa Ephemeral environment spinning up at http://54.190.44.153:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa
Copy link
Member

@jinghua-qa jinghua-qa commented Mar 12, 2022

Thank you for the fix! @stephenLYZ, i verified that the native filter issue has fixed. But i found another issue that when there is color scheme or customized label applied, if user open chart in explore, user will see warning that form data is lost and the color theme or label for the chart will be lost.

Screen.Recording.2022-03-12.at.12.14.55.AM.mov

@stephenLYZ
Copy link
Contributor Author

@stephenLYZ stephenLYZ commented Mar 14, 2022

@jinghua-qa It seems to be related to the recent form_data key update, is there any context for this? @geido @michael-s-molina

@michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Mar 14, 2022

@stephenLYZ I think I know what's the problem here. This PR changed the default cache type to NullCache when not in debug mode. I think this is affecting the test environments. @villebro do you know how can we change this config for test environments?

@jinghua-qa
Copy link
Member

@jinghua-qa jinghua-qa commented Mar 16, 2022

I tested this PR locally, found another issue that, when applied native filter in different series of the same color, after filtered the it will still show same color of different series, is this acceptable?

Screen.Recording.2022-03-15.at.9.37.38.PM.mov

@michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Mar 16, 2022

I tested this PR locally, found another issue that, when applied native filter in different series of the same color, after filtered the it will still show same color of different series, is this acceptable?

@jinghua-qa I think this is the expected behavior and objective of this PR. If you check before/after videos you'll see the same example.

Copy link
Member

@jinghua-qa jinghua-qa left a comment

Executed the following test plan for color consistency and did not find major issues:
https://docs.google.com/spreadsheets/d/1W-sv4LM9XpRttHvMgL-FyWnjaftDGANx9jTCLVlKZUw/edit#gid=1323280743
LGTM!

@rusackas
Copy link
Member

@rusackas rusackas commented Mar 18, 2022

@geido do you want to give this a final code pass? Sounds like we're about to have a beast of a problem behind us!

@geido
Copy link
Member

@geido geido commented Mar 18, 2022

Spinning up another test env. The previous one does not work for me

@geido
Copy link
Member

@geido geido commented Mar 18, 2022

/testenv up

@github-actions
Copy link

@github-actions github-actions bot commented Mar 18, 2022

@geido Ephemeral environment spinning up at http://54.218.201.167:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido
Copy link
Member

@geido geido commented Mar 18, 2022

Hello @stephenLYZ after testing an existing dashboard with a color scheme and no custom label colors, I found that some labels are not sharing the same colors even if they should?

FCC.New.Coder.Survey.2018.mp4

@stephenLYZ
Copy link
Contributor Author

@stephenLYZ stephenLYZ commented Mar 18, 2022

@geido Nice catch! Fixed here.
image

@geido
Copy link
Member

@geido geido commented Mar 18, 2022

/testenv up

@github-actions
Copy link

@github-actions github-actions bot commented Mar 18, 2022

@geido Ephemeral environment spinning up at http://50.112.38.121:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido
Copy link
Member

@geido geido commented Mar 18, 2022

@stephenLYZ I am worried that something happened to the ability to bring the colors of a chart from its Dashboard scheme to Explore. As you can see the colors in Explore look different from the Dashboard.

untitled.dashboard.mp4

@stephenLYZ
Copy link
Contributor Author

@stephenLYZ stephenLYZ commented Mar 19, 2022

@geido Yes, it's a known issue and it only exists in the test environment, see Michael's comment. But in local env it cannot be reproduced.

@stephenLYZ I think I know what's the problem here. This #18976 changed the default cache type to NullCache when not in debug mode. I think this is affecting the test environments. @villebro do you know how can we change this config for test environments?

@zhaoyongjie zhaoyongjie self-requested a review Mar 21, 2022
Copy link
Contributor

@zhaoyongjie zhaoyongjie left a comment

LGTM, Tested in local. work as expected.

@zhaoyongjie zhaoyongjie merged commit dc57508 into apache:master Mar 21, 2022
26 checks passed
@github-actions
Copy link

@github-actions github-actions bot commented Mar 21, 2022

Ephemeral environment shutdown and build artifacts deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants