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: Loading overlay bugfix and cleanup #10105

Merged
merged 11 commits into from Jun 24, 2020

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Jun 18, 2020

SUMMARY

Initial report:

"Filter widgets don't show a ""loading"" spinner, so there is no indication of when the (valid) filter values are loaded. This is a problem with dynamic filters where the underlying logic that returns the filter values can change, or where they depend on settings for other values. The only way to see when it's been updated is by examining the drop-down values to see when they've changed (or if you know what to expect, when the correct values are shown).

Issue uncovered:
The loader was indeed there, but buried (z-index-wise) below the controls.

The fix:
Could this be fixed with one line of CSS? Yep! But that's not the right way. This PR does the following:

  1. Reverses the rendering order of the three key layers in chart.jsx so the Loader is atop the Refresh overlay atop the Chart itself. Not sure why it would be the other way, but if anyone has some idea, let me know!

  2. Touches up a bunch of CSS to make the positioning of those re-stacked elements work

  3. Moves the CSS out of LESS and into Emotion 👩‍🎤

  4. Migrates Loading and RefreshChartOverlay components to tsx.

  5. Removes the size prop from Loading, which sure seems to be unused

  6. Removes some unused css for a "dismiss" button that no longer seems to exist.

BEFORE

captured (4)

AFTER

Jun-18-2020 16-30-58

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue: Preset SO-553
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas changed the title Loading overlay bugfix and cleanup fix: Loading overlay bugfix and cleanup Jun 18, 2020
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple comments, thanks for fixing this!

superset-frontend/src/components/Loading.tsx Outdated Show resolved Hide resolved
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple comments, thanks for fixing this!

@villebro villebro added the v0.37 label Jun 19, 2020
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.

Awesome, works like a charm!

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2020

Codecov Report

Merging #10105 into master will decrease coverage by 0.09%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10105      +/-   ##
==========================================
- Coverage   70.56%   70.46%   -0.10%     
==========================================
  Files         590      590              
  Lines       31150    31209      +59     
  Branches     3164     3192      +28     
==========================================
+ Hits        21981    21992      +11     
- Misses       9060     9101      +41     
- Partials      109      116       +7     
Flag Coverage Δ
#cypress 53.44% <88.88%> (-0.39%) ⬇️
#javascript 59.58% <38.46%> (+0.04%) ⬆️
#python 70.19% <ø> (+<0.01%) ⬆️
Impacted Files Coverage Δ
...et-frontend/src/SqlLab/components/TableElement.jsx 82.75% <0.00%> (ø)
superset-frontend/src/components/Button.jsx 88.88% <ø> (ø)
...et-frontend/src/components/RefreshChartOverlay.tsx 90.00% <80.00%> (ø)
superset-frontend/src/chart/Chart.jsx 66.66% <100.00%> (ø)
superset-frontend/src/components/Loading.tsx 100.00% <100.00%> (ø)
superset-frontend/src/reduxUtils.ts 70.88% <0.00%> (-8.87%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 61.57% <0.00%> (-5.25%) ⬇️
superset-frontend/src/SqlLab/reducers/sqlLab.js 39.09% <0.00%> (-1.65%) ⬇️
...rontend/src/SqlLab/components/TabbedSqlEditors.jsx 80.95% <0.00%> (-0.87%) ⬇️
superset/security/manager.py 88.77% <0.00%> (-0.27%) ⬇️
... and 43 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 02fee35...5371c75. Read the comment docs.

@rusackas
Copy link
Member Author

@etr2460 This PR is like pulling a loose thread on a sweater... I simplified a few more things, by consolidating some global LESS into the component, and removing the className in favor of additional "position" options. I hope this all makes sense. It seems simpler to me, anyway. I think the only recognizable visual diff is that the "inline" style is 10px narrower, which looks better to my eye, in the one place it's used:

Screenshot_2020-06-22_22_51_01

@rusackas rusackas requested a review from etr2460 June 23, 2020 20:00
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm with the exception of the one cleanup line

superset-frontend/src/SqlLab/components/TableElement.jsx Outdated Show resolved Hide resolved
@nytai
Copy link
Member

nytai commented Jun 24, 2020

@rusackas rusackas force-pushed the loading-overlay-bugfix-and-cleanup branch from d9f598b to 22f0381 Compare June 24, 2020 20:55
@rusackas rusackas merged commit 36ea42f into apache:master Jun 24, 2020
@rusackas rusackas deleted the loading-overlay-bugfix-and-cleanup branch June 24, 2020 21:21
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jun 24, 2020
* fix: reordering DOM output, simplifying styles, Emotionalizing

* simplification

* converting RefreshChartOverlay to TS

* Loading -> TS, stripping unused size prop

* simplification...

* just letting "position" prop act as a class name. Simpler!

* consolidating styles, changing a className prop to a position prop.

* nixing (unused) classname prop

* replacing inline loading img with the proper Loading component

* BY THERE.

* position prop is optional!

(cherry picked from commit 36ea42f)
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* fix: reordering DOM output, simplifying styles, Emotionalizing

* simplification

* converting RefreshChartOverlay to TS

* Loading -> TS, stripping unused size prop

* simplification...

* just letting "position" prop act as a class name. Simpler!

* consolidating styles, changing a className prop to a position prop.

* nixing (unused) classname prop

* replacing inline loading img with the proper Loading component

* BY THERE.

* position prop is optional!
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.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 size/L v0.37 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants