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: Change downloadAsImage to use Superset theme #22011

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

lyndsiWilliams
Copy link
Member

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:

dlAsImgBefore

AFTER:

dlAsImgAfter

TESTING INSTRUCTIONS

  • Open a chart and click the dropdown menu in the header next to the save button
  • Hover over the Download option and click "Download as Image" in the resulting menu
  • Observe the background color

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

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #22011 (34ab7e7) into master (429f246) will increase coverage by 1.27%.
The diff coverage is n/a.

@@            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              
Flag Coverage Δ
hive 52.87% <ø> (ø)
javascript 53.50% <ø> (+<0.01%) ⬆️
mysql 78.38% <ø> (ø)
postgres 78.44% <ø> (ø)
presto 52.77% <ø> (ø)
python 81.54% <ø> (+2.61%) ⬆️
sqlite 76.90% <ø> (ø)
unit 51.18% <ø> (?)

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

Impacted Files Coverage Δ
superset-frontend/src/utils/downloadAsImage.ts 11.11% <ø> (-4.68%) ⬇️
...perset-frontend/src/addSlice/AddSliceContainer.tsx 59.61% <0.00%> (ø)
...d/src/filters/components/Time/TimeFilterPlugin.tsx 0.00% <0.00%> (ø)
...ore/components/controls/DateFilterControl/types.ts 100.00% <0.00%> (ø)
...ents/controls/DateFilterControl/utils/constants.ts 100.00% <0.00%> (ø)
superset/connectors/sqla/models.py 91.21% <0.00%> (+0.17%) ⬆️
superset/databases/api.py 93.53% <0.00%> (+0.30%) ⬆️
superset/sql_lab.py 82.12% <0.00%> (+0.38%) ⬆️
superset/views/base_api.py 98.44% <0.00%> (+0.38%) ⬆️
superset/db_engine_specs/presto.py 87.78% <0.00%> (+0.40%) ⬆️
... and 57 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@eschutho eschutho left a 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.

@rusackas
Copy link
Member

rusackas commented Nov 2, 2022

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 #f5f5f5 as the variable added alludes to. Checking your screenshots, it looks like before and after are both the same color here (slightly blue) but I think the before screenshot would/should be #f5f5f5, right? I think that rather than changing this value to one based on primary (which might be any random color when we get into theming) I would go for the closest supported grayscale color. In the theme, that looks like grascale.light4 or #f7f7f7.

Let me know if that all makes sense :)

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@lyndsiWilliams lyndsiWilliams merged commit ba65f66 into master Nov 3, 2022
@lyndsiWilliams lyndsiWilliams deleted the lyndsi/fix-downloadAsImage-theme branch November 3, 2022 02:24
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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 size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants