Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(explore): Implement metrics and columns popovers empty states #18681

Merged
merged 3 commits into from
Feb 14, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Feb 11, 2022

SUMMARY

This PR implements chart empty states for 4 scenarios:

  1. No saved metrics
  2. No saved expressions
  3. No saved temporal expressions
  4. No temporal columns

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

image

image

image

TESTING INSTRUCTIONS

  1. Create a chart using dataset without saved metrics, saved expressions and temporal columns
  2. Check empty states in metrics popover, columns popover and temporal column popover

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

CC @kasiazjc

https://app.shortcut.com/preset/story/36403/explore-empty-states

@kgabryje kgabryje requested a review from villebro February 11, 2022 16:46
@kgabryje kgabryje force-pushed the feat/popovers-empty-stated branch from 775a04c to 1f4dc97 Compare February 11, 2022 17:01
@kgabryje kgabryje requested a review from rusackas February 11, 2022 17:01
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.

Two minor comments, other than that LGTM

image="empty.svg"
title={
isTemporal
? t('No time columns found')
Copy link
Member

Choose a reason for hiding this comment

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

nit: time column sounds like it refers to TIME (hh:mm:ss) as opposed to DATE/DATETIME/TIMESTAMP.

Suggested change
? t('No time columns found')
? t('No temporal columns found')

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

{isTemporal && simpleColumns.length === 0 ? (
<EmptyStateSmall
image="empty.svg"
title={t('No time columns found')}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kgabryje kgabryje force-pushed the feat/popovers-empty-stated branch from 1f4dc97 to 44c8b1d Compare February 14, 2022 09:38
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #18681 (d5fcb17) into master (8d6aff3) will increase coverage by 0.01%.
The diff coverage is 13.33%.

❗ Current head d5fcb17 differs from pull request most recent head 6b4d6e0. Consider uploading reports for the commit 6b4d6e0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18681      +/-   ##
==========================================
+ Coverage   66.32%   66.33%   +0.01%     
==========================================
  Files        1619     1619              
  Lines       62939    62946       +7     
  Branches     6341     6347       +6     
==========================================
+ Hits        41742    41755      +13     
+ Misses      19545    19537       -8     
- Partials     1652     1654       +2     
Flag Coverage Δ
javascript 51.31% <13.33%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx 35.89% <ø> (ø)
...nd/src/explore/components/DataTablesPane/index.tsx 73.68% <0.00%> (-1.59%) ⬇️
...ols/DndColumnSelectControl/ColumnSelectPopover.tsx 3.75% <0.00%> (-0.20%) ⬇️
...ColumnSelectControl/ColumnSelectPopoverTrigger.tsx 65.51% <ø> (ø)
...ontrols/DndColumnSelectControl/DndColumnSelect.tsx 38.09% <ø> (ø)
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 76.28% <50.00%> (-2.07%) ⬇️
superset-frontend/src/explore/constants.ts 100.00% <100.00%> (ø)
...erset-frontend/src/components/EmptyState/index.tsx 35.89% <0.00%> (+35.89%) ⬆️

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 8d6aff3...6b4d6e0. Read the comment docs.

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.

LGTM!

@kgabryje kgabryje merged commit c1205b5 into apache:master Feb 14, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants