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): fix color schemes #13945

Merged
merged 3 commits into from
Apr 4, 2021
Merged

Conversation

simcha90
Copy link
Contributor

@simcha90 simcha90 commented Apr 4, 2021

SUMMARY

This PR fix bug: #13335
That not saved chosen color scheme

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-04-04.at.14.16.17.mov

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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 Apr 4, 2021

Codecov Report

Merging #13945 (76d4fa7) into master (abd4051) will decrease coverage by 2.86%.
The diff coverage is 66.66%.

❗ Current head 76d4fa7 differs from pull request most recent head 91e54d9. Consider uploading reports for the commit 91e54d9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13945      +/-   ##
==========================================
- Coverage   78.03%   75.16%   -2.87%     
==========================================
  Files         934      620     -314     
  Lines       47349    22106   -25243     
  Branches     5943     5777     -166     
==========================================
- Hits        36950    16617   -20333     
+ Misses      10254     5345    -4909     
+ Partials      145      144       -1     
Flag Coverage Δ
hive ?
javascript 66.32% <66.66%> (+<0.01%) ⬆️
mysql ?
postgres ?
presto ?
python ?
sqlite ?

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

Impacted Files Coverage Δ
...ntend/src/dashboard/components/PropertiesModal.jsx 74.10% <66.66%> (+16.01%) ⬆️
superset/commands/importers/exceptions.py
superset/security/__init__.py
superset/db_engine_specs/drill.py
superset/annotation_layers/annotations/dao.py
superset/datasets/metrics/commands/exceptions.py
superset/db_engine_specs/pinot.py
superset/security/api.py
superset/charts/commands/exceptions.py
superset/db_engine_specs/firebird.py
... and 328 more

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 abd4051...91e54d9. Read the comment docs.

@amitmiran137 amitmiran137 merged commit e062906 into apache:master Apr 4, 2021
@amitmiran137 amitmiran137 deleted the fix_colors branch April 4, 2021 12:53
@junlincc junlincc added the dashboard:colors Related to the color scheme of the Dashboard label Apr 6, 2021
@junlincc
Copy link
Member

junlincc commented Apr 6, 2021

thank you @simcha90! i almost missed it. tested on master, LGTM!!
@geido
lmk what you think about this fix. :)

@rusackas @zhaoyongjie Let's put our stamp on it as well.

@geido
Copy link
Member

geido commented Apr 6, 2021

@simcha90 nice that you made CategoricalColorNamespace to work. I struggled to make it work when I attempted to solve this issue.

However, I would like to point out that the system is still defective in a few ways. For the points below, design input will most likely be required. That's why I thought we had better chances to fix the functionality as a whole in the roadmap item apache-superset/superset-roadmap#110

Defects:

  • When changing the label colors in the Advanced - JSON Metadata the changes do not take effect immediately, even after saving. You must refresh the page to see the changes.

  • Any change made to a color scheme in the Advanced - JSON Metadata will be lost when the user selects another color scheme, saves, and refreshes. I am not sure if this is intended to work this way, but it might be the case that a better user experience would be to persist those changes.

CC @junlincc

lyndsiWilliams pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2021
* fix: fix color schemes

* fix: tests case
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* fix: fix color schemes

* fix: tests case
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels dashboard:colors Related to the color scheme of the Dashboard size/S 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants