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(dashboard-css): make to stay custom css when reload #19084

Merged
merged 3 commits into from
Mar 16, 2022

Conversation

prosdev0107
Copy link
Contributor

@prosdev0107 prosdev0107 commented Mar 9, 2022

SUMMARY

Dashboard CSS disappears

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
The dashboard custom css is disappeared when reloaded.
AFTER:
The dashboard custom css stay when reloaded.

TESTING INSTRUCTIONS

How to reproduce the bug

  1. create a dashboard
  2. Edit CSS and save, for example with:

body { background-color: black; }

3.The background appears normally but when you reload the page it is not there and the CSS is gone

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

@rusackas
Copy link
Member

Unit tests seem to be failing on some styling expectation. Can you check to see if those are working locally?

@prosdev0107 prosdev0107 force-pushed the fix/39284-dashboard-css branch 2 times, most recently from 1a2925a to f72acbe Compare March 10, 2022 15:22
@prosdev0107
Copy link
Contributor Author

prosdev0107 commented Mar 10, 2022

@rusackas
The unit test issue was happened because removed calling injectCustomCss from HeaderActionsDropdown.tsx.
I fixed the unit test issue of HeaderActionsDropdown.test.tsx by adding injectCustomCSS function.
I am wondering if it should has the test of rendering the custom css in HeaderActionsDropdown.test.tsx.

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #19084 (e447d19) into master (c79ee56) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #19084   +/-   ##
=======================================
  Coverage   66.51%   66.51%           
=======================================
  Files        1645     1645           
  Lines       63494    63514   +20     
  Branches     6459     6464    +5     
=======================================
+ Hits        42230    42249   +19     
- Misses      19592    19593    +1     
  Partials     1672     1672           
Flag Coverage Δ
javascript 51.27% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
.../components/Header/HeaderActionsDropdown/index.jsx 71.42% <ø> (-0.51%) ⬇️
...-frontend/src/dashboard/reducers/dashboardState.js 73.33% <0.00%> (-3.42%) ⬇️
...erset-frontend/src/components/EmptyState/index.tsx 61.53% <0.00%> (ø)
...t-frontend/src/dashboard/actions/dashboardState.js 36.06% <0.00%> (+0.11%) ⬆️
superset-frontend/src/components/Chart/Chart.jsx 54.09% <0.00%> (+0.76%) ⬆️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 64.67% <0.00%> (+1.63%) ⬆️
...veFilters/FilterBar/FilterControls/FilterValue.tsx 60.43% <0.00%> (+3.46%) ⬆️
...et-frontend/src/components/Chart/ChartRenderer.jsx 52.63% <0.00%> (+7.73%) ⬆️

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 c79ee56...e447d19. Read the comment docs.

@rusackas
Copy link
Member

I am wondering if it should has the test of rendering the custom css in HeaderActionsDropdown.test.tsx.

Ideally all fix:() PRs will have a good test to make sure that whatever's fixed doesn't regress again. If we can add that test in this PR to make sure styles are properly persisted, that'd be fanastic!

@prosdev0107
Copy link
Contributor Author

prosdev0107 commented Mar 11, 2022

@rusackas
That test was already added. I think that this PR can be approved now.

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.

Thanks for the followup. LGTM!

@rusackas rusackas merged commit 30c97ad into apache:master Mar 16, 2022
@sadpandajoe
Copy link
Contributor

🏷️ preset:2022.11

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Mar 17, 2022
* fix(dashboard-css): make to stay custome css when reload

* fix(dashboard-css): make to add injectCustomCSS into HeaderActionsDropdown.test.tsx

(cherry picked from commit 30c97ad)
sadpandajoe added a commit to preset-io/superset that referenced this pull request Mar 23, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
* fix(dashboard-css): make to stay custome css when reload

* fix(dashboard-css): make to add injectCustomCSS into HeaderActionsDropdown.test.tsx

(cherry picked from commit 30c97ad)
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 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 lts-v1 preset:2022.11 size/XS 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants