-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
feat(dashboard): Add thumbnails to dashboard edit draggable chart list #20528
feat(dashboard): Add thumbnails to dashboard edit draggable chart list #20528
Conversation
@kasiazjc here's the PR! |
Codecov Report
@@ Coverage Diff @@
## master #20528 +/- ##
==========================================
- Coverage 66.27% 66.25% -0.02%
==========================================
Files 1757 1757
Lines 66955 67012 +57
Branches 7109 7132 +23
==========================================
+ Hits 44374 44401 +27
- Misses 20766 20785 +19
- Partials 1815 1826 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
superset-frontend/src/dashboard/components/AddSliceCard/index.jsx
Outdated
Show resolved
Hide resolved
@codyml please rebase master to fix the code coverage issue. |
7107355
to
8814886
Compare
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.
looks great!
/testenv up FEATURE_THUMBNAILS=true |
I think we need to make some layout adjustments here... I'll ping you on a separate thread to save everyone the notification headaches :) |
@kgabryje Ephemeral environment spinning up at http://35.90.100.84:8080. Credentials are |
I can't get chart snapshot on Dashboard edit list, but gets it on Charts List. chart.snapshot.in.Dashboard.mov |
Ah thanks, will look into this. |
6598979
to
8ae8e97
Compare
@zhaoyongjie Finally got around to looking into this, and it's another symptom of the missing metadata bug, which I'm going to tackle separately. TBD if this is going to be the final fix, but if you comment out this line you should see all thumbnails and metadata appear. |
/testenv up FEATURE_THUMBNAILS=true |
@rusackas Ephemeral environment spinning up at http://54.202.124.179:8080. Credentials are |
3ed59af
to
6255fc0
Compare
this PR should wait for merging this. |
6255fc0
to
361e406
Compare
@zhaoyongjie #20673 was merged! Mind merging this if everything still looks good? |
/testenv up FEATURE_THUMBNAILS=true |
@geido Ephemeral environment spinning up at http://34.210.42.74:8080. Credentials are |
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.
@@ -0,0 +1,276 @@ | |||
/** |
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.
Just noticed this is a JSX, we should migrate to TSX for the new component.
@geido found an issue with metadata still being missing, need to look into it: |
/testenv up FEATURE_THUMBNAILS=true |
/testenv up FEATURE_THUMBNAILS=true FEATURE_THUMBNAILS_SQLA_LISTENERS=true |
@zhaoyongjie Ephemeral environment spinning up at http://35.91.99.142:8080. Credentials are |
1 similar comment
@zhaoyongjie Ephemeral environment spinning up at http://35.91.99.142:8080. Credentials are |
b4f9311
to
8557c7c
Compare
/testenv up FEATURE_THUMBNAILS=true FEATURE_THUMBNAILS_SQLA_LISTENERS=true |
@zhaoyongjie Ephemeral environment spinning up at http://54.185.197.92:8080. Credentials are |
/testenv up FEATURE_THUMBNAILS=true |
@zhaoyongjie Ephemeral environment spinning up at http://18.236.83.241:8080. Credentials are |
@geido @zhaoyongjie Confirmed that missing metadata in ephemeral environment was due to ephemeral environment being spun up before PR was rebased over #20684. |
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
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This PR adds thumbnails to the sidebar of draggable charts in the edit dashboard view and modifies the design of the cards with and without thumbnails enabled. Thumbnails are shown when the
THUMBNAILS
feature flag is enabled. If no thumbnail has been generated, the same placeholder that's used on the chart list card view is shown. Other changes include:Added
badge, when thumbnails are enabled and when they aren't.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Before.mov
After (thumbnails enabled):
Untitled.mov
After (thumbnails disabled):
Untitled2.mov
TESTING INSTRUCTIONS
Note: Thumbnail generation is currently broken (see #20673). In addition, there's a bug preventing the chart metadata from being downloaded (see #20684), so even if thumbnails are generated, any items in the draggable chart list with missing metadata won't show thumbnails either.
THUMBNAILS=False
and ensure that the updated cards look and work as expected when added and not added to the dashboard.THUMBNAILS=True
and ensure that thumbnails appear (or at least that their placeholders do) as expected and that the updated cards look and work as expected when added and not added to the dashboard.ADDITIONAL INFORMATION
THUMBNAILS