refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBuilder #11160

kgabryje · 2020-10-05T11:50:36Z

SUMMARY

This PR replaces usages of react-bootstrap's Tabs component with tabs from antd in DashboardBuilder. As you can see on the gifs, besides the new look I also removed the popover with delete button. Now it should be more intuitive.
On the downside, drag'n'drop functionality is a bit less handy - you need to drag a tab to the edge of another tab's title rather than the edge of tab itself. It worked the same in the old version, but due to different UI it was more noticeable. Changing this behaviour would require a major refactor - I think we should do it, but as a part of a different task/PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

ADDITIONAL INFORMATION

kgabryje · 2020-10-05T11:51:17Z

@rusackas Can you please take a look?

mistercrunch · 2020-10-07T14:51:38Z

Should we use the other style of antd tabs instead of these? Looks like antd offers 2-3 different looks here https://ant.design/components/tabs/. Let's get designer input on which one to use here and elsewhere.

kgabryje · 2020-10-07T15:11:11Z

@mistercrunch I think it's a great idea to ask for a designer's input, but let's keep in mind that in the case of editable tabs (with add and remove options) we have no choice and we need to use the "card" style. However in the case of non-editable tabs we can use either "line" or "card" style.

mistercrunch · 2020-10-07T15:21:48Z

Depending on designers input, we may be able to switch the tab style in edit/view mode if necessary too. @kgabryje is it because the "non-card" tabs don't allow for arbitrary React node? They force you to pass a string?

kgabryje · 2020-10-07T16:14:42Z

Contrary to react-bootstrap tabs, here we use Antd's native functionality of adding and removing tabs, and it's required to pass type=editable-card for it to work. Of course we can switch styles between "line" and "card" depending on edit mode, but here I opted for keeping card style for consistency.

rusackas · 2020-10-08T18:14:19Z

@kgabryje I posed this question to designers (admittedly, internal ones... the broader community is welcome to engage here!), and got some feedback:

The technical constraints, requiring the use of the card-style tabs here are acknowledged. The design perspective, however, is that we should limit the number of visible tab styles in our design system. The preferred approach would be to use these card-based tabs, but restyle them so they have a more consistent appearance with the underlined tabs we use elsewhere.

I don't believe it should block this PR, as I think this PR is a big step in the right direction. A PR in which we re-style the various AntD tab flavors to be more consistent seems like a worthy follow-up task.

Curious how others feel about that proposal.

ktmud · 2020-10-08T19:03:12Z

Not a designer, but I also feel the original design is a cleaner and better user experience. It'd be a little weird to have different visual styles (line vs card) for tabs in View and in Edit modes.

In general, we should probably limit style changes when replacing Bootstrap components. For this case, it'd be easier and less risky if we had just kept the original editing functionalities and only replaced the BootstrapTabs component itself.

codecov-io · 2020-10-15T17:09:32Z

Codecov Report

Merging #11160 into master will decrease coverage by 7.36%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master   #11160      +/-   ##
==========================================
- Coverage   66.58%   59.21%   -7.37%     
==========================================
  Files         873      830      -43     
  Lines       41767    40206    -1561     
  Branches     3827     3577     -250     
==========================================
- Hits        27809    23807    -4002     
- Misses      13856    16231    +2375     
- Partials      102      168      +66

Flag	Coverage Δ
cypress	`55.55% <77.27%> (+0.08%)`	⬆️
javascript	`?`
python	`61.18% <ø> (-0.91%)`	⬇️

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

Impacted Files	Coverage Δ
...tend/src/dashboard/components/DashboardBuilder.jsx	`85.91% <0.00%> (ø)`
...d/src/dashboard/components/gridComponents/Tabs.jsx	`65.59% <71.87%> (-10.73%)`	⬇️
...erset-frontend/src/common/components/Tabs/Tabs.tsx	`96.00% <100.00%> (-0.30%)`	⬇️
...set-frontend/src/components/URLShortLinkButton.jsx	`100.00% <100.00%> (ø)`
...nd/src/dashboard/components/gridComponents/Tab.jsx	`67.27% <100.00%> (-5.20%)`	⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js	`0.00% <0.00%> (-100.00%)`	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	`0.00% <0.00%> (-100.00%)`	⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx	`0.00% <0.00%> (-100.00%)`	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	`0.00% <0.00%> (-93.48%)`	⬇️
... and 301 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 5d9448f...90f22a6. Read the comment docs.

rusackas · 2020-10-20T04:13:44Z

superset-frontend/stylesheets/superset.less

 .dashboard-component.dashboard-component-header .anchor-link-container {
  .fa.fa-link {
    font-size: @font-size-l;
  }
 }

-.nav.nav-tabs li:hover,
+.ant-tabs-nav-list .ant-tabs-tab {


Would much rather see this moved into Emotion, in the Tabs component, and using gridUnit * 3 instead of 12

rusackas · 2020-10-20T04:16:17Z

superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tabs_spec.jsx

@@ -113,7 +108,7 @@ describe('Tabs', () => {
    const createComponent = sinon.spy();
    const wrapper = setup({ editMode: true, createComponent });
    wrapper
-      .find('.dashboard-component-tabs .nav-tabs a')
+      .find('.dashboard-component-tabs .ant-tabs-nav-add')


might be worth considering a data-test attribute here, in case our class names change for library/styling purposes.

rusackas · 2020-10-20T04:18:20Z

superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tabs_spec.jsx

-        <WithDragDropContext>
-          <Tabs {...props} {...overrideProps} />
-        </WithDragDropContext>
+        <ThemeProvider theme={supersetTheme}>


If you'd rather not import the supersetTheme and ThemeProvider to use here, you can instead import { styledMount as mount } from 'spec/helpers/theming' and then just do

<Provider store={mockStoreWithTabs}> <WithDragDropContext> <Tabs {...props} {...overrideProps} /> </WithDragDropContext> </Provider>

rusackas · 2020-10-20T04:22:48Z

In the last conversation I had with designers about this, the conclusion was:
• We should use the same style of tabs in presentation and edit mode
• Therefore, if we need to use the card-based tabs in edit mode, then we should use that style of tabs in both modes.
• It would be great if we can then style those tabs to make them look like the underlined AntD tabs we're starting to add to other views.
• If that styling change needs to happen in another PR, so we can get this one across the finish line, so be it.

Any objections to the above?

kgabryje · 2020-10-28T13:24:29Z

In the last conversation I had with designers about this, the conclusion was:
• We should use the same style of tabs in presentation and edit mode
• Therefore, if we need to use the card-based tabs in edit mode, then we should use that style of tabs in both modes.
• It would be great if we can then style those tabs to make them look like the underlined AntD tabs we're starting to add to other views.
• If that styling change needs to happen in another PR, so we can get this one across the finish line, so be it.

Any objections to the above?

Thank you, I'll restyle the tabs to make them look more like "line" style tabs.

kgabryje · 2020-10-28T20:21:28Z

New look of the tabs in DashboardBuilder:

@rusackas @ktmud Thoughts?

kgabryje · 2020-11-02T12:01:08Z

I did a rebase to resolve conflicts. Also, I moved styled from tabs.less to Emotion and uppercased editable title input for consistency with other tabs modes.

suddjian

looks good to me, happy to merge if others' concerns are addressed.

rusackas

LGTM

…lder (apache#11160) * Replace tabs in DashboardBuilder * Fix tests * Fix styling of anchor * Fix * Fix cypress test * Fix tests * Fix e2e tests * Use data-tests * Move tabs styles from superset.less to Emotion * Restyle tabs in DashboardBuilder * Test fix * Fix styling

superset-github-bot bot added the polidea label Oct 5, 2020

pull-request-size bot added the size/L label Oct 5, 2020

willbarrett requested a review from nytai October 5, 2020 22:30

mistercrunch added the need:design-review Requires input/approval from a Designer label Oct 7, 2020

rusackas requested a review from suddjian October 7, 2020 20:32

kgabryje mentioned this pull request Oct 15, 2020

refactor: Replace react-bootstrap tabs with Antd tabs #11118

Merged

6 tasks

kgabryje force-pushed the antd/tabs3 branch from 0d58760 to 7af0b46 Compare October 15, 2020 16:29

kgabryje closed this Oct 15, 2020

kgabryje reopened this Oct 15, 2020

kgabryje force-pushed the antd/tabs3 branch from 28b8e89 to a855669 Compare October 16, 2020 14:33

rusackas reviewed Oct 20, 2020

View reviewed changes

kgabryje force-pushed the antd/tabs3 branch from fd66ecb to 1c65f50 Compare October 28, 2020 20:20

kgabryje force-pushed the antd/tabs3 branch from 19f0ea2 to eac5be3 Compare October 29, 2020 08:58

kgabryje added 4 commits November 2, 2020 10:57

Replace tabs in DashboardBuilder

953504b

Fix tests

8c7c33b

Fix styling of anchor

d6eed87

Fix

8d6f79c

kgabryje added 7 commits November 2, 2020 10:57

Fix cypress test

6714459

Fix tests

47a7789

Fix e2e tests

c1782b4

Use data-tests

f9cd07f

Move tabs styles from superset.less to Emotion

27ecb4e

Restyle tabs in DashboardBuilder

d209177

Test fix

ace17ed

kgabryje force-pushed the antd/tabs3 branch from eac5be3 to ace17ed Compare November 2, 2020 10:07

Fix styling

90f22a6

kgabryje closed this Nov 2, 2020

kgabryje reopened this Nov 2, 2020

suddjian approved these changes Nov 2, 2020

View reviewed changes

rusackas approved these changes Nov 2, 2020

View reviewed changes

rusackas merged commit a874b14 into apache:master Nov 2, 2020

graceguo-supercat mentioned this pull request Nov 12, 2020

[dashboard] Can not remove row inside Tab #11681

Closed

3 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBuilder #11160

refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBuilder #11160

kgabryje commented Oct 5, 2020 •

edited

Loading

kgabryje commented Oct 5, 2020

mistercrunch commented Oct 7, 2020 •

edited

Loading

kgabryje commented Oct 7, 2020

mistercrunch commented Oct 7, 2020

kgabryje commented Oct 7, 2020

rusackas commented Oct 8, 2020

ktmud commented Oct 8, 2020

codecov-io commented Oct 15, 2020 •

edited

Loading

rusackas Oct 20, 2020

rusackas Oct 20, 2020

rusackas Oct 20, 2020

rusackas commented Oct 20, 2020

kgabryje commented Oct 28, 2020

kgabryje commented Oct 28, 2020

kgabryje commented Nov 2, 2020

suddjian left a comment

rusackas left a comment

refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBuilder #11160

refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBuilder #11160

Conversation

kgabryje commented Oct 5, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

kgabryje commented Oct 5, 2020

mistercrunch commented Oct 7, 2020 • edited Loading

kgabryje commented Oct 7, 2020

mistercrunch commented Oct 7, 2020

kgabryje commented Oct 7, 2020

rusackas commented Oct 8, 2020

ktmud commented Oct 8, 2020

codecov-io commented Oct 15, 2020 • edited Loading

Codecov Report

rusackas Oct 20, 2020

Choose a reason for hiding this comment

rusackas Oct 20, 2020

Choose a reason for hiding this comment

rusackas Oct 20, 2020

Choose a reason for hiding this comment

rusackas commented Oct 20, 2020

kgabryje commented Oct 28, 2020

kgabryje commented Oct 28, 2020

kgabryje commented Nov 2, 2020

suddjian left a comment

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

kgabryje commented Oct 5, 2020 •

edited

Loading

mistercrunch commented Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 15, 2020 •

edited

Loading