[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135

cchaos · 2021-09-02T19:45:18Z

The style update to EuiTabs should only affect Amsterdam (the default theme will still behave as it did).

Deprecated `display` as a distinguishing prop for EuiTabs in Amsterdam

I was doing some auditing of the different styles we had of EuiTabs in Figma (especially across the different embedded components like flyouts and page headers) and decided we had too many variants and to consolidate it down.

Essentially we had 3 sizes (s | m | l) and two displays (default | condensed). The condensed version removed the bottom border, made the text bolder, and shrunk the displayed width of each tab (narrower bottom border for active tab).

But I was also seeing places where the condensed version was being used with a bottom border supplied by a parent component, like the page header:

So it was getting confusing to know when to choose default vs condensed style tabs. So I just decided to remove this option in place of just showing/hiding the bottomBorder. Which reduced the style to just setting size:

This also means, however, that all the tabs (in Amsterdam), are now bold. I find this to be a better approach as they are more distinguishable and easier to find. Though maybe down the line (with Emotion) we can make that changeable.

Added `bottomBorder` prop

Now this bottom border is easily removed by simply toggling this prop (which is true by default).

Screen.Recording.2021-09-02.at.03.31.05.PM.mp4

Added `xl` size option

Our page template allows using tabs as the actual page title, but when used as the page title even at size l, they were pretty small. So I added the xl option with a prop comment about only using this size when specifically using as a page title.

I also updated EuiPageHeaderContent to use this new size when a pageTitle is not present:

Tabs as the only page header content

I continued the page header update to make the layout a bit better when they are the only content. Instead of have a space separating the underline of the tab and the page contents, I increased the overall height of the xl tabs to stretch the height of the page header so that it sat nicely on the page header's bottom border.

I also fixed the a11y of the missing h1 by using the label of the currently selected tab in a screen-reader-only <h1>.

Added `prepend` and `append` props to tabs

Closes #5057

These props accept a simple node, but also forced me to move the text styling of the tabs to the .euiTab__content element so as to not interfere with the node. The docs examples also are now using those to display an icon and notification badge.

Other notes

I re-worked the Sass of the tabs to use inset borders instead of pseudo elements which is a cleaner approach and fixes the responsive/overflow behavior where if it scrolled, the bottom border would scroll away too.
[Breaking?]: I removed the associated tab Sass variables for font sizing. Only one of the 3 was actually being used and I decided to remove them completely because the tab sizing is based on more than just font size.
While I was in EuiPageHeaderContents, and I had remembered this request from downstream teams, I also added pageTitleProps so consumers can pass extra props like data-test-subj to the EuiTitle wrapper.
I touched a lot of docs files. I converted what I could to .tsx and mostly re-wrote the Tabs and Page Header pages.

Checklist

and deprecated `display`

And Docs updates

…tling

kibanamachine · 2021-09-02T20:10:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

kibanamachine · 2021-09-02T21:11:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

# Conflicts: # src-docs/src/views/tabs/tabs_example.js

kibanamachine · 2021-09-03T20:04:32Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

Accepts a simple node but forced the text styles to move to the `.euiTab__content` element so as to not interfere with the node.

kibanamachine · 2021-09-03T21:43:24Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

kibanamachine · 2021-09-07T21:17:26Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

cee-chen · 2021-09-08T00:36:25Z

I'm planning on reviewing this PR tomorrow! I'm still not yet super familiar with/an expert on determining what's a breaking change vs. a non breaking change, but I'd say if the prop deprecation isn't one, then removing tab Sass variables likely isn't a huge deal (btw, have we checked to make sure it's not being used in Kibana?)

That being said, this PR could likely piggy back on the 38.0 release needed for the screen reader PR in any case!

cee-chen

Loved the various cleanup! Source code and QA looked great to me, I mostly had just questions and a few copy/typo comments.

src/components/tabs/tabs.tsx

src/components/tabs/tab.test.tsx

src/components/page/page_header/page_header_content.tsx

src-docs/src/views/tabs/tabs_example.js

src-docs/src/views/tabs/tabs_flyout.tsx

src-docs/src/views/tabs/tabs.tsx

src-docs/src/views/tabs/playground.js

src-docs/src/views/tabs/controlled.tsx

src-docs/src/views/page_header/page_header_example.js

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

…Props`

kibanamachine · 2021-09-08T18:23:40Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

cchaos · 2021-09-08T19:35:45Z

src-docs/src/views/tabs/controlled.tsx

 } from '../../../../src/components';

-const tabs = [
+const tabs: EuiTabbedContentProps['tabs'] = [


Ugh, so this is where the CodeSandbox link WILL break because of TS (defining types). It mostly just ignores it as function params, but here I get a full blown error, no render. But removing the type here has a cascade effect and really this is the best way to implement this consumer-side.

I'm open to suggestions.

Again, it was actually very important to do this in the docs because it did catch an error where the EuiTabbedContentProps needed to fully extend the EuITabProps in order to pass down the new props I added.

I'm looking into this locally now and I think I should be able to get CodeSandbox to build .tsx rather than .js files, which makes this demo work! The only thing is, I'm leaning towards making that its own PR instead of lumping it into this one (since it's already so big). WDYT? I'd personally be OK merging this PR in as-is (with a temporarily broken CodeSandbox link) knowing that we'll fix that functionality in a fast follow-up PR.

🙌 Yeah if a fix is incoming for that, we def don't need to bloat or block this PR with it. Thank you!

kibanamachine · 2021-09-08T20:01:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

cee-chen

Changes look fantastic! I should have a PR for Codesandbox TSX support in a little bit here for you to test!

cee-chen · 2021-09-08T21:09:20Z

I'll wait for this PR to land before opening a new PR, but I have a commit you can check out locally to confirm it works: cee-chen@9afba51

Implementation-wise, we should be able to convert specific code demos to load in .tsx in CodeSandBox as-needed (which seems to match other 'update as we go' docs patterns).

miukimiu

Looks great! 😍

It's a great idea to showing/hiding the bottomBorder instead of choosing default or condensed. The prepend and append make implementing icons and badges much easier and consistent.

I looked at the code and @constance did a very good job finding some issues. Tested in Chrome, Safari, Edge, Firefox and everything worked well. ✌🏽

kibanamachine · 2021-09-09T14:27:49Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/

cchaos added 9 commits September 1, 2021 18:47

[EuiTabs] Refactoring styles, added bottomBorder

c4a6f0b

and deprecated `display`

Docs

7ab27e1

Added size xl and cleaned up styles

fddf277

[EuiPageHeader] Changed layout if tabs is the only content

6872c0f

And Docs updates

Fixing up page_body style

0161a2f

Docs cleanup

a96074a

[a11y] Added a visually hidden h1 when only tabs are used for page ti…

12b560a

…tling

Added pageTitleProps to allow passing more options to the page title

5ea3205

Revert change to template doc

e38a7de

Merge remote-tracking branch 'upstream/master' into ams/tabs

a693ea4

# Conflicts: # src-docs/src/views/tabs/tabs_example.js

cchaos added 4 commits September 3, 2021 17:06

Added prepend / append props

2382337

Accepts a simple node but forced the text styles to move to the `.euiTab__content` element so as to not interfere with the node.

cl

5badd84

cl

d105418

cl

7f9a686

Cleanup

e19d31d

cee-chen self-requested a review September 8, 2021 00:32

cchaos requested a review from miukimiu September 8, 2021 13:49

cchaos added the deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. label Sep 8, 2021

cee-chen reviewed Sep 8, 2021

View reviewed changes

cchaos and others added 3 commits September 8, 2021 13:38

Constance catching all my typos 😅

1340136

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

Merge branch 'master' into ams/tabs

c485129

Fixed type of EuiTabbedContentTab to extend everything from `EuiTab…

a43bc0c

…Props`

Update docs about bottomBorder

493ed17

cchaos commented Sep 8, 2021

View reviewed changes

Using useMemo for tab content in example

aa31884

cee-chen approved these changes Sep 8, 2021

View reviewed changes

miukimiu approved these changes Sep 9, 2021

View reviewed changes

Merge branch 'elastic:master' into ams/tabs

e268c02

cchaos enabled auto-merge (squash) September 9, 2021 14:06

cchaos merged commit 0badbf2 into elastic:master Sep 9, 2021

cee-chen mentioned this pull request Sep 9, 2021

[Docs] Add support for .tsx CodeSandbox demos #5160

Merged

2 tasks

cchaos deleted the ams/tabs branch September 9, 2021 21:16

1Copenut mentioned this pull request Sep 16, 2021

Bumping EUI to 38.0.1 elastic/kibana#112462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135

[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135

cchaos commented Sep 2, 2021 •

edited

Loading

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 7, 2021

cee-chen commented Sep 8, 2021

cee-chen left a comment

kibanamachine commented Sep 8, 2021

cchaos Sep 8, 2021

cee-chen Sep 8, 2021

cchaos Sep 8, 2021

kibanamachine commented Sep 8, 2021

cee-chen left a comment

cee-chen commented Sep 8, 2021 •

edited

Loading

miukimiu left a comment

kibanamachine commented Sep 9, 2021

[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135

[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135

Conversation

cchaos commented Sep 2, 2021 • edited Loading

Deprecated display as a distinguishing prop for EuiTabs in Amsterdam

Added bottomBorder prop

Added xl size option

Tabs as the only page header content

Added prepend and append props to tabs

Other notes

Checklist

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 7, 2021

cee-chen commented Sep 8, 2021

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 8, 2021

cchaos Sep 8, 2021

Choose a reason for hiding this comment

cee-chen Sep 8, 2021

Choose a reason for hiding this comment

cchaos Sep 8, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 8, 2021

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 8, 2021 • edited Loading

miukimiu left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 9, 2021

cchaos commented Sep 2, 2021 •

edited

Loading

Deprecated `display` as a distinguishing prop for EuiTabs in Amsterdam

Added `bottomBorder` prop

Added `xl` size option

Added `prepend` and `append` props to tabs

cee-chen commented Sep 8, 2021 •

edited

Loading