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: Apply custom label colors in the Dashboard immediately #16637

Closed
wants to merge 8 commits into from
Closed

fix: Apply custom label colors in the Dashboard immediately #16637

wants to merge 8 commits into from

Conversation

geido
Copy link
Member

@geido geido commented Sep 8, 2021

SUMMARY

This PR fixes an issue with the custom label colors not being applied on the fly.

BEFORE

Please check issue #11677

AFTER

Test.dashboard.2.mp4

TESTING INSTRUCTIONS

  1. Open a Dashboard
  2. Go to "Edit properties"
  3. Got to "Advanced"
  4. Change the label colors
  5. Save and observe the behavior

ADDITIONAL INFORMATION

  • Has associated issue: Fixes [dashboard properties] metadata saving behavior on SAVE #11677
  • 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

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #16637 (805bf58) into master (a6173f1) will increase coverage by 0.00%.
The diff coverage is 82.35%.

❗ Current head 805bf58 differs from pull request most recent head 3f9ea84. Consider uploading reports for the commit 3f9ea84 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #16637   +/-   ##
=======================================
  Coverage   76.92%   76.92%           
=======================================
  Files        1031     1031           
  Lines       55151    55165   +14     
  Branches     7498     7504    +6     
=======================================
+ Hits        42426    42438   +12     
- Misses      12473    12474    +1     
- Partials      252      253    +1     
Flag Coverage Δ
javascript 70.89% <82.35%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/chart/Chart.jsx 43.33% <ø> (ø)
superset-frontend/src/chart/ChartRenderer.jsx 36.70% <0.00%> (-0.48%) ⬇️
.../src/dashboard/components/gridComponents/Chart.jsx 80.37% <ø> (ø)
...shboard/util/charts/getFormDataWithExtraFilters.ts 83.87% <50.00%> (ø)
...et-frontend/src/dashboard/actions/dashboardInfo.ts 60.00% <88.88%> (+12.94%) ⬆️
...perset-frontend/src/dashboard/containers/Chart.jsx 93.75% <100.00%> (+1.44%) ⬆️
...uperset-frontend/src/explore/exploreUtils/index.js 66.87% <100.00%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6173f1...3f9ea84. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

A few minor non-blocking comments. Ping @kgabryje - I wonder if this will reintroduce colors into the URL that was removed in #16621 ?

superset-frontend/src/dashboard/actions/dashboardInfo.ts Outdated Show resolved Hide resolved
superset-frontend/src/dashboard/containers/Chart.jsx Outdated Show resolved Hide resolved
@kgabryje
Copy link
Member

kgabryje commented Sep 9, 2021

@geido @villebro Unfortunately it seems that this change might contribute even more to the problems with very long explore URLs.
I tested it using Slack dashboard and Messages per Channel chart.
This is the url from my local environment:
image
And this is the one from testenv spinned for this PR:
image

@geido
Copy link
Member Author

geido commented Sep 9, 2021

Thanks for spotting this issue @villebro @kgabryje . I believe this can be handled differently to avoid the consequences with the URL length. I'll work on that and ping you again

@geido
Copy link
Member Author

geido commented Oct 7, 2021

@kgabryje @villebro what I found is that even in the current implementation the label_colors should be polluting the URL but in some cases the scale is just returning empty, that's the reason why it seems that the URL isn't affected.
The correct solution here is to remove the label_colors from the formData and pass them directly to the chart so that the URL won't ever be polluted. This is the change that I just did.

It remains the problem of how we pass the label colors to the chart when coming from the Dashboard to Explore. However, that is a much bigger problem and will need to be tackled in a separate PR.

@geido geido requested review from villebro and kgabryje and removed request for kgabryje October 7, 2021 13:04
@kgabryje
Copy link
Member

kgabryje commented Oct 8, 2021

Are you sure that label_colors were removed from URL? I went from Slack dashboard to Messages per Channel chart in Explore and it seems that label colors are included in URL.
On the left is my localhost built on the latest master, on the right test env from this PR
image

@apache apache deleted a comment from kgabryje Oct 8, 2021
@apache apache deleted a comment from github-actions bot Oct 8, 2021
@apache apache deleted a comment from pkdotson Oct 8, 2021
@apache apache deleted a comment from github-actions bot Oct 8, 2021
@geido
Copy link
Member Author

geido commented Oct 8, 2021

@kgabryje you are perfectly right but this was a funny one. Basically, the reason why label_colors do not pollute the URL on master is due to that bug I mentioned before. The bug was removed with my changes and I did not notice that I had to forcefully remove the label_colors from the URL as well, which is what I just did in my latest changes. It should be good to go now I hope!

@geido
Copy link
Member Author

geido commented Oct 11, 2021

/testenv up

@github-actions
Copy link
Contributor

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

@geido
Copy link
Member Author

geido commented Oct 13, 2021

Closing this in favour of #17089 which is going to cover all the other cases

@geido geido closed this Oct 13, 2021
@github-actions
Copy link
Contributor

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
Development

Successfully merging this pull request may close these issues.

[dashboard properties] metadata saving behavior on SAVE
4 participants