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: data column in SQL lab left panel open by default #13624

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

AAfghahi
Copy link
Member

SUMMARY

Fix to Left Panel tables being auto-expanded when added. This was a feature they had previous to the switch to antd Collapse.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-03-15.at.10.32.11.AM.mov

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #13624 (b78eba4) into master (a35825d) will decrease coverage by 4.19%.
The diff coverage is 100.00%.

❗ Current head b78eba4 differs from pull request most recent head a168a78. Consider uploading reports for the commit a168a78 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13624      +/-   ##
==========================================
- Coverage   77.21%   73.02%   -4.20%     
==========================================
  Files         912      608     -304     
  Lines       46449    21639   -24810     
  Branches     5725     5725              
==========================================
- Hits        35866    15801   -20065     
+ Misses      10447     5702    -4745     
  Partials      136      136              
Flag Coverage Δ
cypress 56.56% <0.00%> (-0.01%) ⬇️
hive ?
javascript 63.04% <100.00%> (+<0.01%) ⬆️
postgres ?
presto ?
python ?
sqlite ?

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/actions/sqlLab.js 63.03% <ø> (ø)
...et-frontend/src/SqlLab/components/TableElement.jsx 88.37% <ø> (ø)
...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx 54.54% <100.00%> (+1.71%) ⬆️
superset/reports/commands/base.py
superset/models/tags.py
superset/constants.py
superset/common/query_object.py
superset/views/css_templates.py
...t/annotation_layers/annotations/commands/update.py
superset/charts/commands/bulk_delete.py
... and 297 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 a35825d...a168a78. Read the comment docs.

Comment on lines +151 to +152
.filter(({ expanded }) => expanded)
.map(({ id }) => id)}
Copy link
Member

Choose a reason for hiding this comment

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

I love Javascript's object unpacking, it's really elegant.

@junlincc
Copy link
Member

@michael-s-molina ^ fyi collapse default behavior is not all the same in the product. i should have caught it. my bad. 🙏

@junlincc junlincc changed the title fix: Left panel fix: data column in SQL lab left panel open by default Mar 17, 2021
@michael-s-molina
Copy link
Member

@michael-s-molina ^ fyi collapse default behavior is not all the same in the product. i should have caught it. my bad. 🙏

No problem at all 😉.

Thank you all for the fix.

@AAfghahi
Copy link
Member Author

AAfghahi commented Mar 17, 2021

@eschutho I was thinking about what the most effective way of testing that expanded is true when a table is added. I decided that checking the reducer would be the most effective way of making sure that when the action is taken, that the new table has an expanded value of true. Is this the correct thinking? Or should I be testing sqlLab action as well?

In the latter case, looking through https://redux.js.org/recipes/writing-tests says that I should be importing the reducer and checking it (that's why I decided to just test the reducer itself). Am I thinking of this correctly?

Lastly --long note sorry-- I was thinking about testing the functionality of adding a column. Currently, I wrote an RTL test that checks to make sure that a table with expanded == true is shown in the document. I wanted to add a test that tests the functionality of clicking the each individual collapse panel (though there is a spec written for the Collapse panel component that checks this, so maybe not necessary).

Also, I wanted to write a test that has you adding a table and that should come in expanded. The problem that I ran into here was that I think that I'd have to write a dataset/schema/table combination and then have fetchMocks that adds them to initial state? Does that sound correct? Would the test reside in SqlLeftEditor bar? Is this test necessary given that the reducer shows that tables come in with expanded == true and that expanded tables show in the document? I want to be thorough but not overzealous.

ahh sorry for the note, been thinking about this a lot.

@hughhhh hughhhh self-requested a review March 18, 2021 20:28
@betodealmeida betodealmeida merged commit df9352f into apache:master Mar 18, 2021
@eschutho eschutho deleted the LeftPanel branch March 19, 2021 23:07
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* fix table expand

* Left Panel Expand

* added tests

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 preset-io size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants