refactor(explore): Enhance Dataset and Control panel Collapse components #12218

geido · 2020-12-28T18:01:06Z

SUMMARY

Removes react-bootstrap Collapse with Antdesign enhanced Collapse.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Collapse_BBEFORE.mp4

After:

Collapse_AFTER.mp4

TEST PLAN

Open the explorer
Make sure the control panel has all the collapsible expanded
Close a collapsible panel, the content should be hidden
Re-open the closed collapsible panel, the content should appear

ADDITIONAL INFORMATION

geido · 2020-12-28T18:28:21Z

@rusackas @junlincc

junlincc · 2021-01-01T22:14:13Z

thanks @geido for the PR. if the caret was the only thing that changed, it LGTM. but im not sure why the tabs in the control panel got messed up a bit...

this is what im seeing and how it supposed to look in another open PR

geido · 2021-01-01T22:16:59Z

Thanks @junlincc. Let me check what might have caused that.

…anced_collapse

geido · 2021-01-04T17:28:20Z

@junlincc Please check now. The issue should be resolved.

junlincc

manually tested, LGTM!

If it's easy, please remove 'Dataset' from the Dataset & Chart Type Section in this PR as well.

and get the tests passed.

…anced_collapse

ktmud · 2021-01-05T19:56:43Z

The caret directions are inconsistent with the datasource panel.

Folded:

Expanded:

I think the directions used by the datasource pane makes more sense.

ktmud · 2021-01-05T19:58:32Z

Also, can we fix the font-size inconsistency in these panes as well (shouldn't they use the same styled component)?

ktmud · 2021-01-05T20:06:41Z

May I also suggest tune the font size/spacing of the columns/metrics as well?

Before:

After:

(a rough adjustment, could be better)

ktmud · 2021-01-05T20:07:24Z

The title border spacing is also inconsistent.

geido · 2021-01-05T20:15:09Z

Hello @ktmud. Thanks for your feedback. We are working on moving the Collapse components to Antd in small steps. This is the reason why you see inconsistencies between the two. We should use Antdesign as a reference though.

Concerning the fonts, react-bootstrap was having a 16px overriding the default 14px. This should be an improvement. Unless you think keeping the 16px imposed by react-bootstrap was intentional.

As for the direction of the arrows, those are by default in Antd. While I agree with you that the directions used by the previous Collapse are more intuitive, this is a change that will have an impact on all the Collapse components already moved to Antd. I would keep it as an item for later.

codecov-io · 2021-01-05T20:25:16Z

Codecov Report

Merging #12218 (88413e2) into master (878f123) will decrease coverage by 3.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12218      +/-   ##
==========================================
- Coverage   66.83%   63.39%   -3.45%     
==========================================
  Files        1022      488     -534     
  Lines       49992    30102   -19890     
  Branches     4892        0    -4892     
==========================================
- Hits        33411    19082   -14329     
+ Misses      16456    11020    -5436     
+ Partials      125        0     -125

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`63.39% <ø> (-0.61%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/db_engine_specs/hive.py	`73.84% <0.00%> (-10.00%)`	⬇️
superset/databases/commands/create.py	`83.67% <0.00%> (-8.17%)`	⬇️
superset/databases/commands/update.py	`85.71% <0.00%> (-8.17%)`	⬇️
superset/connectors/sqla/models.py	`84.31% <0.00%> (-6.28%)`	⬇️
superset/db_engine_specs/sqlite.py	`90.62% <0.00%> (-6.25%)`	⬇️
superset/databases/commands/test_connection.py	`84.78% <0.00%> (-4.35%)`	⬇️
superset/utils/celery.py	`96.42% <0.00%> (-3.58%)`	⬇️
superset/models/core.py	`85.59% <0.00%> (-3.27%)`	⬇️
... and 541 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 878f123...88413e2. Read the comment docs.

ktmud · 2021-01-05T22:16:41Z

Cool. Glad to hear this is an ongoing migration. Just documenting what I found here in case they slip through.

geido · 2021-01-06T11:00:57Z

After another look, I have noticed that the DatasourcePanel is using the Antd Collapse already, just not the enhanced one that we have introduced in the common components, but another flavor of it. This will need to be consolidated. Most probably all the existing instances of the Antd Collapse that aren't using the enhanced one will need to be moved to the enhanced. Thoughts @rusackas?

rusackas · 2021-01-22T20:09:02Z

After much discussion with the Preset design team (other voices can pitch in here!) we think we'd like to keep the current arrow directions.

As far as arrow direction goes, the design proposal is this: If the collapse arrow is on the right side of the content area (like controls), it will point up when open or down when closed.. If it's on the left side of a header (like the Data pane) it will point right when closed, and down when open.

The above is more or less in accordance with the Apple guidelines.

We should utilize a prop in the Collapse component to pick which version we're using... it appears the expandIconPosition could trigger conditional styling.

There's also a discrepancy in animation - we agreed that they should all be animated (and quickly!) with clockwise rotation when opening, and counterclockwise when closing.

And in coming to all these conclusions, we came up with a whole system of floating design proposals as Github Issues, then moving them to the Github wiki when consensus is reached. We'll be sending a proposal around this to the dev@ list soon!

ktmud · 2021-01-22T21:30:16Z

While we are here, can we also fix the click area?

Currently the Dateset panel accordions can click to fold/expand on the whole header section, but the Controls panel accordions only have the text and the caret clickable.

ktmud · 2021-01-22T21:35:41Z

Personally I'm also not a fan of caret animation (and even the slide up/slide down animation maybe we can also get rid of).

Airbnb design guidelines for internal tools is avoid animations. We don't even have animation for modals and popovers.

For tools people will use everyday, (the presumed) efficiency and speed are probably more important than the "pleasant surprises" animations are normally designed to inspire.

geido · 2021-01-22T21:54:27Z

While we are here, can we also fix the click area?

@ktmud this PR will bring that for free as the Antdesign Collapse has the full header area clickable

geido · 2021-01-22T21:59:28Z

@rusackas @ktmud I have made some adjustments so that the Dataset panel and Control panels use the same styling. As for the open points, concerning animation and direction of the arrows, let's come to a consensus so that I can close this up.

I tend to agree to avoid animations. As for the direction of the arrows, I am fine with what Evan has proposed.

…anced_collapse

geido · 2021-01-25T15:38:46Z

I have made more changes as follows:

Added an animateArrows prop so that those animations can be optional
Implemented the arrow open/close position based on whether the expandIconPosition is right or left
Added a storybook entry to show the arrows when the animation is ON
I have enhanced also the DataSource panel, so it looks the same as the Control panel
I have also updated the description with the updated videos showing the functionality

This is quite a big change. I will appreciate support with testing eventual edge cases.

Thank you!

@junlincc @rusackas @ktmud

superset-frontend/src/common/components/Collapse.tsx

ktmud

LGTM. Hope we can have a Collapse.test.tsx file but that's a low priority.

adam-stasiak · 2021-01-25T19:13:19Z

I see that when I click on second collapse then field with search metrics and columns jumps a little bit.
Are we able to fix this?

Nagranie.z.ekranu.2021-01-25.o.20.03.12.mov

geido · 2021-01-25T19:23:56Z

Hello @adam-stasiak I am seeing the same problem on master. Would you like to create a separate issue for that?

https://www.awesomescreenshot.com/video/2508610?key=292778fadfc41d2f0f20f3099117aea6

Thanks

adam-stasiak · 2021-01-25T19:25:15Z

ah ok! I will create! sorry for inconvenience.

adam-stasiak · 2021-01-25T20:46:34Z

after manual testing looks fine to me 🟢

* master: (52 commits) docs: Updates to Superset Site for 1.0 (apache#12626) test(native-filters): scoping tree in native filters modal (apache#12655) Fix tests errors and warnings - iteration 3 (apache#12212) (apache#12219) Fix tests errors and warnings - iteration 5 (apache#12212) (apache#12224) Fix tests errors and warnings - iteration 6 (apache#12212) (apache#12227) feat(native-filters): apply scoping of native filters to dashboard (apache#12716) Fix tests errors and warnings - iteration 4 (apache#12212) (apache#12223) Fix tests errors and warnings - iteration 7 (apache#12212) (apache#12245) fix: missing select menu background (apache#12759) fix(explore): incorrect missing datasource condition (apache#12758) feat: default timepicker to last week when dataset is changed (apache#12609) feat(explore): allow opening charts with missing dataset (apache#12705) chore: upgrade Cypress to 6.2.1 (apache#12605) refactor(explore): Enhance Dataset and Control panel Collapse components (apache#12218) feat: Adding option to set_database_uri CLI command (apache#12740) docs: Fixed typo on line 348 (apache#12739) Fix tests errors and warnings - iteration 2 (apache#12212) (apache#12214) docs: Remove gatsby-plugin-offline (apache#12693) test: oracle engine spec (apache#12615) test: hive db engine spec (apache#12520) ...

Enhance control panel Collapse

836f271

pull-request-size bot added the size/L label Dec 28, 2020

geido closed this Dec 31, 2020

geido reopened this Dec 31, 2020

junlincc added the assigned:flexiana label Jan 1, 2021

junlincc requested review from ktmud, rusackas and etr2460 January 1, 2021 21:57

mistercrunch closed this Jan 4, 2021

mistercrunch reopened this Jan 4, 2021

geido added 2 commits January 4, 2021 16:56

Merge remote-tracking branch 'upstream/master' into control_panel_enh…

a3ff229

…anced_collapse

Fix wrong import and minor refactor

fa78171

junlincc self-requested a review January 5, 2021 01:21

junlincc approved these changes Jan 5, 2021

View reviewed changes

geido added 2 commits January 5, 2021 18:52

Fix for missing expanded param

b079ec3

Merge remote-tracking branch 'upstream/master' into control_panel_enh…

65222f2

…anced_collapse

rusackas added the hold! On hold label Jan 6, 2021

geido mentioned this pull request Jan 6, 2021

[Explore]Data panel Metrics section should expand on click the caret #12233

Closed

Fix merge conflicts

e76b583

junlincc added need:design-review Requires input/approval from a Designer and removed hold! On hold labels Jan 22, 2021

Apply consistent styling - in progress

7aa95ee

Align DataSource panel to enhanced component

fdc7366

Clean up DataSource panels style

00b355f

geido changed the title ~~refactor: Enhance control panel Collapse~~ refactor(explore): Enhance Dataset and Control panel Collapse components Jan 22, 2021

Fix merge conflicts

9da9e0f

junlincc added the explore:refactor Related to refactoring Explore label Jan 23, 2021

geido added 3 commits January 25, 2021 14:34

Merge remote-tracking branch 'upstream/master' into control_panel_enh…

9029505

…anced_collapse

Add optional animateArrows prop

1f24242

Add storybook entry

a7e9152

maloun96 reviewed Jan 25, 2021

View reviewed changes

superset-frontend/src/common/components/Collapse.tsx Show resolved Hide resolved

ktmud approved these changes Jan 25, 2021

View reviewed changes

Remove negative margin

88413e2

ktmud merged commit 1b2611c into apache:master Jan 25, 2021

villebro mentioned this pull request May 6, 2021

feat(explore): collapse time section if no ts columns #14493

Merged

8 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(explore): Enhance Dataset and Control panel Collapse components #12218

refactor(explore): Enhance Dataset and Control panel Collapse components #12218

geido commented Dec 28, 2020 •

edited

Loading

geido commented Dec 28, 2020

junlincc commented Jan 1, 2021 •

edited

Loading

geido commented Jan 1, 2021 •

edited

Loading

geido commented Jan 4, 2021

junlincc left a comment

ktmud commented Jan 5, 2021

ktmud commented Jan 5, 2021 •

edited

Loading

ktmud commented Jan 5, 2021

ktmud commented Jan 5, 2021

geido commented Jan 5, 2021

codecov-io commented Jan 5, 2021 •

edited

Loading

ktmud commented Jan 5, 2021

geido commented Jan 6, 2021

rusackas commented Jan 22, 2021

ktmud commented Jan 22, 2021

ktmud commented Jan 22, 2021 •

edited

Loading

geido commented Jan 22, 2021

geido commented Jan 22, 2021

geido commented Jan 25, 2021

ktmud left a comment

adam-stasiak commented Jan 25, 2021

geido commented Jan 25, 2021

adam-stasiak commented Jan 25, 2021

adam-stasiak commented Jan 25, 2021

refactor(explore): Enhance Dataset and Control panel Collapse components #12218

refactor(explore): Enhance Dataset and Control panel Collapse components #12218

Conversation

geido commented Dec 28, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

geido commented Dec 28, 2020

junlincc commented Jan 1, 2021 • edited Loading

geido commented Jan 1, 2021 • edited Loading

geido commented Jan 4, 2021

junlincc left a comment

Choose a reason for hiding this comment

ktmud commented Jan 5, 2021

ktmud commented Jan 5, 2021 • edited Loading

ktmud commented Jan 5, 2021

ktmud commented Jan 5, 2021

geido commented Jan 5, 2021

codecov-io commented Jan 5, 2021 • edited Loading

Codecov Report

ktmud commented Jan 5, 2021

geido commented Jan 6, 2021

rusackas commented Jan 22, 2021

ktmud commented Jan 22, 2021

ktmud commented Jan 22, 2021 • edited Loading

geido commented Jan 22, 2021

geido commented Jan 22, 2021

geido commented Jan 25, 2021

ktmud left a comment

Choose a reason for hiding this comment

adam-stasiak commented Jan 25, 2021

geido commented Jan 25, 2021

adam-stasiak commented Jan 25, 2021

adam-stasiak commented Jan 25, 2021

geido commented Dec 28, 2020 •

edited

Loading

junlincc commented Jan 1, 2021 •

edited

Loading

geido commented Jan 1, 2021 •

edited

Loading

ktmud commented Jan 5, 2021 •

edited

Loading

codecov-io commented Jan 5, 2021 •

edited

Loading

ktmud commented Jan 22, 2021 •

edited

Loading