-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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: Change downloadAsImage to use Superset theme #22011
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22011 +/- ##
==========================================
+ Coverage 65.76% 67.03% +1.27%
==========================================
Files 1813 1813
Lines 69437 69424 -13
Branches 7450 7448 -2
==========================================
+ Hits 45667 46541 +874
+ Misses 21850 20963 -887
Partials 1920 1920
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but it could probably use another set of eyes from @rusackas or @michael-s-molina since they may know this portion of the codebase better.
I applaud your effort in reinforcing proper theme usage! I also randomly enjoyed a little trip down the commit history that this led to! Regarding the comment you refer to in the code and its link to line 34, this gets interesting. At the time of the PR adding that comment to the codebase, the referenced line 34 actually pointed to a value that was indeed Let me know if that all makes sense :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
SUMMARY
This PR changes the background color of downloadAsImage to use the Superset theme. Originally, the value for this color was held in a local variable that referenced this line from our LESS variables file, but was assigned to the color
#F5F5F5
. Since that referenced color is now#d2edf4
in our LESS variables file, I referenced the matching color in our Superset theme instead (supersetTheme.colors.primary.light3
).BEFORE/AFTER SCREENSHOTS
I downloaded a chart as an image for both of these screenshots
BEFORE:
AFTER:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION