-
Notifications
You must be signed in to change notification settings - Fork 862
[EuiTabs] Consolidated styling options for Amsterdam and updated usage in EuiPageHeader #5135
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
Conversation
and deprecated `display`
And Docs updates
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
1 similar comment
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
# Conflicts: # src-docs/src/views/tabs/tabs_example.js
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
|
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loved the various cleanup! Source code and QA looked great to me, I mostly had just questions and a few copy/typo comments.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
| } from '../../../../src/components'; | ||
|
|
||
| const tabs = [ | ||
| const tabs: EuiTabbedContentProps['tabs'] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 Yeah if a fix is incoming for that, we def don't need to bloat or block this PR with it. Thank you!
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
cee-chen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look fantastic! I should have a PR for Codesandbox TSX support in a little bit here for you to test!
|
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 |
elizabetdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ✌🏽
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5135/ |
The style update to EuiTabs should only affect Amsterdam (the default theme will still behave as it did).
Deprecated
displayas a distinguishing prop for EuiTabs in AmsterdamI 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
condensedversion 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 settingsize: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
bottomBorderpropNow this bottom border is easily removed by simply toggling this prop (which is
trueby default).Screen.Recording.2021-09-02.at.03.31.05.PM.mp4
Added
xlsize optionOur 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 thexloption 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
pageTitleis 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
xltabs 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
h1by using the label of the currently selected tab in a screen-reader-only<h1>.Added
prependandappendprops to tabsCloses #5057
These props accept a simple node, but also forced me to move the text styling of the tabs to the
.euiTab__contentelement 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
pageTitlePropsso consumers can pass extra props likedata-test-subjto the EuiTitle wrapper..tsxand mostly re-wrote the Tabs and Page Header pages.Checklist