fix(sql Lab tabs): Empty SQL Lab tabs #18817

AAfghahi · 2022-02-18T20:46:08Z

SUMMARY

This PR finishes the work that was done in #17700 by adding a new tab if a user exits out of their current tabs.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

Screen.Recording.2022-02-18.at.3.49.05.PM.mov

TESTING INSTRUCTIONS

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

eschutho · 2022-02-18T21:09:50Z

/testenv up

github-actions · 2022-02-18T21:11:36Z

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

hughhhh · 2022-02-18T21:29:26Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx

+    const emptyTab = (
+      <EditableTabs.TabPane
+        key={Math.random()}
+        data-key={Math.random()}


Do we need to have these key and data-key set?

Yeah I was worried about hardcoding a number, but I think that you are right and that this would be the ebst way forward.

hughhhh · 2022-02-18T21:33:25Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx

+        closable={false}
+      >
+        <StyledImage>
+          <img src={emptySqlChart} alt="No SQL tabs" />


can you write a small test making sure this renders when this.props.queryEditors?.length === 0

codecov · 2022-02-18T21:38:16Z

Codecov Report

Merging #18817 (b867472) into master (5752372) will increase coverage by 0.02%.
The diff coverage is 75.00%.

❗ Current head b867472 differs from pull request most recent head 01d1199. Consider uploading reports for the commit 01d1199 to get more accurate results

@@            Coverage Diff             @@
##           master   #18817      +/-   ##
==========================================
+ Coverage   66.19%   66.21%   +0.02%     
==========================================
  Files        1633     1633              
  Lines       63210    63218       +8     
  Branches     6409     6412       +3     
==========================================
+ Hits        41839    41858      +19     
+ Misses      19711    19699      -12     
- Partials     1660     1661       +1

Flag	Coverage Δ
javascript	`51.02% <75.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...d/src/SqlLab/components/TabbedSqlEditors/index.jsx	`57.44% <75.00%> (+1.05%)`	⬆️
superset-frontend/src/chart/Chart.jsx	`51.78% <0.00%> (ø)`
superset-frontend/src/chart/chartReducer.ts	`25.00% <0.00%> (ø)`
superset/views/core.py	`77.50% <0.00%> (+0.44%)`	⬆️
superset/models/core.py	`88.91% <0.00%> (+0.72%)`	⬆️
superset/db_engine_specs/mysql.py	`97.59% <0.00%> (+3.61%)`	⬆️
superset/common/utils/dataframe_utils.py	`92.85% <0.00%> (+7.14%)`	⬆️

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 5752372...01d1199. Read the comment docs.

yousoph

Can we update the tab height in the blank state so that it doesn't jump when you add a new tab?
The image on the blank state looks blurry to me right now, do you mind taking a look?

villebro · 2022-02-21T06:25:32Z

Thanks for working on this @AAfghahi! I pinged @kgabryje for a review; He's done similar work by introducing a new EmptyState component in #18676 which has been rolled out in several follow-up PRs (#18678, #18679, #18681 and #18712). By leveraging a similar approach here we could potentially address @yousoph's observation regarding the blurry empty state and consolidate code that deals with empty states.

kgabryje

Hey @AAfghahi! 3 comments:

Can you please use 1 of the components from src/components/EmptyState? I think EmptyStateBig will fit well here.
I'd suggest using svg instead of png. If you add the image to /superset-frontend/src/assets/images/, you can just pass the name of the file to the empty state component.
Please export from figma just the image, without the text. EmptyState accepts title and description props

So you can add the empty state component like this:

<EmptyStateBig
          title={t('Add a new tab to create SQL query')}
          image="image-name.svg"
/>

kgabryje · 2022-02-21T14:19:39Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx

+      <EditableTabs.TabPane
+        key={0}
+        data-key={0}
+        tab={<TabTitle>Add a New Tab</TabTitle>}


Suggested change

tab={<TabTitle>Add a New Tab</TabTitle>}

tab={<TabTitle>{t('Add a New Tab')}</TabTitle>}

Also, I think we should use sentence case here, like Add a new tab

Thank you for this and the empty state, implemented them, just working on the last bit (making the tabs the right height).

kgabryje

Thanks for the improvements!
1 more comment - the text in empty tab doesn't seem to be centered

AAfghahi · 2022-02-22T19:54:55Z

Thanks for the improvements! 1 more comment - the text in empty tab doesn't seem to be centered

good catch! I need to add some padding to make it the same height as regular tab. This should be fixed now.

AAfghahi · 2022-02-23T22:28:21Z

/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=True

github-actions · 2022-02-23T22:32:44Z

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

jinghua-qa

LGTM!

spoke to her about it

github-actions · 2022-02-25T18:48:38Z

Ephemeral environment shutdown and build artifacts deleted.

* Empty SQL table message on zero tabs * sql editor no editor tab bug fix * Revert Error message * empty state tab state * added a unit test * addressed reviews * kasia feedback Co-authored-by: Yahya Kayani <yahyakiani1@gmail.com> (cherry picked from commit 147dc5a)

superset-github-bot bot added the Superset-Community-Partners Preset community partner program participants label Feb 18, 2022

pull-request-size bot added the size/S label Feb 18, 2022

AAfghahi force-pushed the arash/empty_sql_tab_state branch 2 times, most recently from a7bf598 to 7438bce Compare February 18, 2022 20:48

AAfghahi changed the title ~~Arash/empty sql tab state~~ fix (sql Lab tabs): Empty SQL Lab tabs Feb 18, 2022

AAfghahi changed the title ~~fix (sql Lab tabs): Empty SQL Lab tabs~~ fix(sql Lab tabs): Empty SQL Lab tabs Feb 18, 2022

hughhhh reviewed Feb 18, 2022

View reviewed changes

hughhhh approved these changes Feb 18, 2022

View reviewed changes

yousoph previously requested changes Feb 18, 2022

View reviewed changes

villebro requested a review from kgabryje February 21, 2022 06:21

kgabryje requested changes Feb 21, 2022

View reviewed changes

kgabryje reviewed Feb 21, 2022

View reviewed changes

hughhhh self-requested a review February 22, 2022 05:29

pull-request-size bot added size/M and removed size/S labels Feb 22, 2022

AAfghahi force-pushed the arash/empty_sql_tab_state branch from 62809d9 to a11d409 Compare February 22, 2022 18:16

pull-request-size bot added size/S and removed size/M labels Feb 22, 2022

AAfghahi force-pushed the arash/empty_sql_tab_state branch from a11d409 to a4755d0 Compare February 22, 2022 18:27

pull-request-size bot added size/M and removed size/S labels Feb 22, 2022

kgabryje requested changes Feb 22, 2022

View reviewed changes

AAfghahi force-pushed the arash/empty_sql_tab_state branch from a4755d0 to 953a53a Compare February 22, 2022 20:55

AAfghahi requested a review from kgabryje February 22, 2022 20:55

eschutho added the need:qa-review Requires QA review label Feb 23, 2022

apache deleted a comment from github-actions bot Feb 23, 2022

AAfghahi requested a review from yousoph February 24, 2022 15:21

AAfghahi force-pushed the arash/empty_sql_tab_state branch from 0b8d956 to 801e499 Compare February 24, 2022 16:46

Yahyakiani and others added 7 commits February 24, 2022 12:10

Empty SQL table message on zero tabs

fbb5d3c

sql editor no editor tab bug fix

b5a42bd

Revert Error message

7ad0128

empty state tab state

5b26074

added a unit test

3d91f2f

addressed reviews

91c4a5e

kasia feedback

01d1199

AAfghahi force-pushed the arash/empty_sql_tab_state branch from 801e499 to 01d1199 Compare February 24, 2022 17:10

jinghua-qa approved these changes Feb 24, 2022

View reviewed changes

hughhhh approved these changes Feb 25, 2022

View reviewed changes

hughhhh merged commit 147dc5a into apache:master Feb 25, 2022

villebro added the lts-v1 label Mar 29, 2022

This was referenced Apr 18, 2022

fix(Sql Lab tabs): Empty SQL lab tabs #17700

Closed

Closing all tabs on SQL Lab leaves the screen blank #17630

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql Lab tabs): Empty SQL Lab tabs #18817

fix(sql Lab tabs): Empty SQL Lab tabs #18817

AAfghahi commented Feb 18, 2022 •

edited

Loading

eschutho commented Feb 18, 2022

github-actions bot commented Feb 18, 2022

hughhhh Feb 18, 2022 •

edited

Loading

AAfghahi Feb 18, 2022

hughhhh Feb 18, 2022

AAfghahi Feb 18, 2022

codecov bot commented Feb 18, 2022 •

edited

Loading

yousoph left a comment

villebro commented Feb 21, 2022

kgabryje left a comment •

edited

Loading

kgabryje Feb 21, 2022

kgabryje Feb 21, 2022 •

edited

Loading

AAfghahi Feb 22, 2022

kgabryje left a comment

AAfghahi commented Feb 22, 2022 •

edited

Loading

AAfghahi commented Feb 23, 2022

github-actions bot commented Feb 23, 2022

jinghua-qa left a comment

github-actions bot commented Feb 25, 2022

	tab={<TabTitle>Add a New Tab</TabTitle>}
	tab={<TabTitle>{t('Add a New Tab')}</TabTitle>}

fix(sql Lab tabs): Empty SQL Lab tabs #18817

fix(sql Lab tabs): Empty SQL Lab tabs #18817

Conversation

AAfghahi commented Feb 18, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

eschutho commented Feb 18, 2022

github-actions bot commented Feb 18, 2022

hughhhh Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

AAfghahi Feb 18, 2022

Choose a reason for hiding this comment

hughhhh Feb 18, 2022

Choose a reason for hiding this comment

AAfghahi Feb 18, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

yousoph left a comment

Choose a reason for hiding this comment

villebro commented Feb 21, 2022

kgabryje left a comment • edited Loading

Choose a reason for hiding this comment

kgabryje Feb 21, 2022

Choose a reason for hiding this comment

kgabryje Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

AAfghahi Feb 22, 2022

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

AAfghahi commented Feb 22, 2022 • edited Loading

AAfghahi commented Feb 23, 2022

github-actions bot commented Feb 23, 2022

jinghua-qa left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 25, 2022

AAfghahi commented Feb 18, 2022 •

edited

Loading

hughhhh Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

kgabryje left a comment •

edited

Loading

kgabryje Feb 21, 2022 •

edited

Loading

AAfghahi commented Feb 22, 2022 •

edited

Loading