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

feat(dashboard): Add thumbnails to dashboard edit draggable chart list #20528

Conversation

codyml
Copy link
Member

@codyml codyml commented Jun 28, 2022

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:

  • Layout updates for the text and the Added badge, when thumbnails are enabled and when they aren't.
  • Text is truncated and shows a tooltip when it's too long to fit in the layout.
  • The human-readable chart viz type is displayed instead of the machine-readable name.

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.

  1. Run Superset with feature flag THUMBNAILS=False and ensure that the updated cards look and work as expected when added and not added to the dashboard.
  2. Run Superset with 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

  • Has associated issue:
  • Required feature flags: THUMBNAILS
  • 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

@codyml
Copy link
Member Author

codyml commented Jun 28, 2022

@kasiazjc here's the PR!

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #20528 (8557c7c) into master (6e6d4e3) will decrease coverage by 0.01%.
The diff coverage is 68.18%.

@@            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     
Flag Coverage Δ
javascript 51.93% <68.18%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...et-frontend/src/dashboard/actions/sliceEntities.js 74.28% <ø> (ø)
...dashboard/components/AddSliceCard/AddSliceCard.tsx 67.44% <67.44%> (ø)
...t-frontend/src/dashboard/components/SliceAdder.jsx 62.66% <100.00%> (ø)
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx 78.04% <0.00%> (-5.74%) ⬇️
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 75.53% <0.00%> (-2.50%) ⬇️
...ColumnSelectControl/ColumnSelectPopoverTrigger.tsx 63.63% <0.00%> (-1.89%) ⬇️
...ols/DndColumnSelectControl/ColumnSelectPopover.tsx 3.26% <0.00%> (-0.49%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@zhaoyongjie
Copy link
Member

@codyml please rebase master to fix the code coverage issue.

@codyml codyml force-pushed the codyml/sc-50980/add-thumbnails-next-to-charts-in-dashboard branch from 7107355 to 8814886 Compare June 29, 2022 13:49
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

looks great!

@kgabryje
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true

@rusackas
Copy link
Member

I think we need to make some layout adjustments here... I'll ping you on a separate thread to save everyone the notification headaches :)

@github-actions
Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://35.90.100.84:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codyml codyml marked this pull request as draft June 29, 2022 16:28
@rusackas rusackas added the hold! On hold label Jun 29, 2022
@zhaoyongjie
Copy link
Member

I can't get chart snapshot on Dashboard edit list, but gets it on Charts List.

chart.snapshot.in.Dashboard.mov

@codyml
Copy link
Member Author

codyml commented Jun 30, 2022

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.

@codyml codyml force-pushed the codyml/sc-50980/add-thumbnails-next-to-charts-in-dashboard branch 4 times, most recently from 6598979 to 8ae8e97 Compare July 8, 2022 23:07
@codyml codyml marked this pull request as ready for review July 9, 2022 03:44
@codyml
Copy link
Member Author

codyml commented Jul 9, 2022

I can't get chart snapshot on Dashboard edit list, but gets it on Charts List.

chart.snapshot.in.Dashboard.mov

@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.

@rusackas rusackas removed the hold! On hold label Jul 12, 2022
@rusackas
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://54.202.124.179:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codyml codyml force-pushed the codyml/sc-50980/add-thumbnails-next-to-charts-in-dashboard branch from 3ed59af to 6255fc0 Compare July 14, 2022 17:29
@zhaoyongjie zhaoyongjie self-requested a review July 15, 2022 11:56
@zhaoyongjie
Copy link
Member

this PR should wait for merging this.

@codyml codyml force-pushed the codyml/sc-50980/add-thumbnails-next-to-charts-in-dashboard branch from 6255fc0 to 361e406 Compare July 19, 2022 19:39
@codyml
Copy link
Member Author

codyml commented Jul 19, 2022

this PR should wait for merging this.

@zhaoyongjie #20673 was merged! Mind merging this if everything still looks good?

@geido
Copy link
Member

geido commented Jul 21, 2022

/testenv up FEATURE_THUMBNAILS=true

@github-actions
Copy link
Contributor

@geido Ephemeral environment spinning up at http://34.210.42.74:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

awesome! I have got the screenshot in the edit bar on the dashboard. leave a no-blocking suggestion, migrate to TSX in separated PR or this PR.

image

@@ -0,0 +1,276 @@
/**
Copy link
Member

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.

@codyml
Copy link
Member Author

codyml commented Jul 22, 2022

@geido found an issue with metadata still being missing, need to look into it:
Screen Shot 2022-07-22 at 10 44 59 AM

@zhaoyongjie
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true

@zhaoyongjie
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true FEATURE_THUMBNAILS_SQLA_LISTENERS=true

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://35.91.99.142:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

1 similar comment
@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://35.91.99.142:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codyml codyml force-pushed the codyml/sc-50980/add-thumbnails-next-to-charts-in-dashboard branch from b4f9311 to 8557c7c Compare July 26, 2022 19:24
@zhaoyongjie
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true FEATURE_THUMBNAILS_SQLA_LISTENERS=true

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://54.185.197.92:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@zhaoyongjie
Copy link
Member

/testenv up FEATURE_THUMBNAILS=true

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://18.236.83.241:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codyml
Copy link
Member Author

codyml commented Jul 27, 2022

@geido @zhaoyongjie Confirmed that missing metadata in ephemeral environment was due to ephemeral environment being spun up before PR was rebased over #20684.

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

@rusackas rusackas merged commit d50784d into apache:master Jul 28, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@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 preset-io size/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants