-
Notifications
You must be signed in to change notification settings - Fork 377
Tabs updates #4146
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
Tabs updates #4146
Changes from all commits
42a1f51
edf976c
c6183a9
2295dc7
2b4bdc5
e9c2c85
ef9a8d3
b2b0b7f
7605433
64a2057
5eb8490
0276937
0261e58
bebcb64
cc782b4
5c853b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import * as React from 'react'; | ||
| import { css } from '@patternfly/react-styles'; | ||
| import styles from '@patternfly/react-styles/css/components/Tabs/tabs'; | ||
|
|
||
| export interface TabTitleIconProps extends React.HTMLProps<HTMLSpanElement> { | ||
| /** Icon to be rendered inside the tab button title. */ | ||
| children: React.ReactNode; | ||
| /** additional classes added to the tab title icon */ | ||
| className?: string; | ||
| } | ||
|
|
||
| export const TabTitleIcon: React.FunctionComponent<TabTitleIconProps> = ({ | ||
| children, | ||
| className = '', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: we don't have to set these to a default of empty string anymore on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I followed the API we have in the drawer component for width. |
||
| ...props | ||
| }: TabTitleIconProps) => ( | ||
| <span className={css(styles.tabsItemIcon, className)} {...props}> | ||
| {children} | ||
| </span> | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import * as React from 'react'; | ||
| import { css } from '@patternfly/react-styles'; | ||
| import styles from '@patternfly/react-styles/css/components/Tabs/tabs'; | ||
|
|
||
| export interface TabTitleTextProps extends React.HTMLProps<HTMLSpanElement> { | ||
| /** Text to be rendered inside the tab button title. */ | ||
| children: React.ReactNode; | ||
| /** additional classes added to the tab title text */ | ||
| className?: string; | ||
| } | ||
|
|
||
| export const TabTitleText: React.FunctionComponent<TabTitleTextProps> = ({ | ||
| children, | ||
| className = '', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: we don't have to set these to a default of empty string anymore on |
||
| ...props | ||
| }: TabTitleTextProps) => ( | ||
| <span className={css(styles.tabsItemText, className)} {...props}> | ||
| {children} | ||
| </span> | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,46 +5,63 @@ import { css } from '@patternfly/react-styles'; | |
| import { PickOptional } from '../../helpers/typeUtils'; | ||
| import AngleLeftIcon from '@patternfly/react-icons/dist/js/icons/angle-left-icon'; | ||
| import AngleRightIcon from '@patternfly/react-icons/dist/js/icons/angle-right-icon'; | ||
| import { getUniqueId, isElementInView } from '../../helpers/util'; | ||
| import { getUniqueId, isElementInView, formatBreakpointMods, capitalize } from '../../helpers/util'; | ||
| import { TabButton } from './TabButton'; | ||
| import { TabContent } from './TabContent'; | ||
| import { getOUIAProps, OUIAProps } from '../../helpers'; | ||
|
|
||
| export enum TabsVariant { | ||
| export enum TabsComponent { | ||
| div = 'div', | ||
| nav = 'nav' | ||
| } | ||
| export interface TabsBreakpointMod { | ||
| /** The attribute to modify */ | ||
| modifier: 'insetNone' | 'insetSm' | 'insetMd' | 'insetLg' | 'insetXl' | 'inset_2xl'; | ||
| /** The breakpoint at which to apply the modifier */ | ||
| breakpoint?: 'md' | 'lg' | 'xl' | '2xl'; | ||
| } | ||
|
|
||
| export interface TabsProps extends Omit<React.HTMLProps<HTMLElement | HTMLDivElement>, 'onSelect'> { | ||
| /** content rendered inside the Tabs Component. */ | ||
| /** Content rendered inside the tabs component. */ | ||
| children: React.ReactNode; | ||
| /** additional classes added to the Tabs */ | ||
| /** Additional classes added to the tabs */ | ||
| className?: string; | ||
| /** the index of the active tab */ | ||
| /** The index of the active tab */ | ||
| activeKey?: number | string; | ||
| /** handle tab selection */ | ||
| /** Callback to handle tab selection */ | ||
| onSelect?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: number | string) => void; | ||
| /** uniquely identifies the Tabs */ | ||
| /** Uniquely identifies the tabs */ | ||
| id?: string; | ||
| /** enables the filled tab list layout */ | ||
| /** Enables the filled tab list layout */ | ||
| isFilled?: boolean; | ||
| /** enables Secondary Tab styling */ | ||
| /** Enables secondary tab styling */ | ||
| isSecondary?: boolean; | ||
| /** Aria-label for the left Scroll Button */ | ||
| /** Enables box styling to the tab component */ | ||
| isBox?: boolean; | ||
| /** Enables vertical tab styling */ | ||
| isVertical?: boolean; | ||
| /** Aria-label for the left scroll button */ | ||
| leftScrollAriaLabel?: string; | ||
| /** Aria-label for the right Scroll Button */ | ||
| /** Aria-label for the right scroll button */ | ||
| rightScrollAriaLabel?: string; | ||
| /** determines what tag is used around the Tabs. Use "nav" to define the Tabs inside a navigation region */ | ||
| variant?: 'div' | 'nav'; | ||
| /** provides an accessible label for the Tabs. Labels should be unique for each set of Tabs that are present on a page. When variant is set to nav, this prop should be defined to differentiate the Tabs from other navigation regions on the page. */ | ||
| /** Determines what tag is used around the tabs. Use "nav" to define the tabs inside a navigation region */ | ||
| component?: 'div' | 'nav'; | ||
| /** Provides an accessible label for the tabs. Labels should be unique for each set of tabs that are present on a page. When component is set to nav, this prop should be defined to differentiate the tabs from other navigation regions on the page. */ | ||
| 'aria-label'?: string; | ||
| /** waits until the first "enter" transition to mount tab children (add them to the DOM) */ | ||
| /** Waits until the first "enter" transition to mount tab children (add them to the DOM) */ | ||
| mountOnEnter?: boolean; | ||
| /** unmounts tab children (removes them from the DOM) when they are no longer visible */ | ||
| /** Unmounts tab children (removes them from the DOM) when they are no longer visible */ | ||
| unmountOnExit?: boolean; | ||
| /* Modifies the tabs component padding/inset to visually match padding of other adjacent components.*/ | ||
| inset?: 'none' | 'sm' | 'md' | 'lg' | 'xl' | '2xl'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like our breakpoint mods helpers would be a better API here? The structure is looking similar.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I experimented with our breakpoint modifier API and I think it'd be a good fit and extendable to future modifiers: export interface TabBreakpointMod {
/** The attribute to modify */
modifier:
| 'insetNone'
| 'insetSm'
| 'insetMd'
| 'insetLg'
| 'insetXl'
| 'inset_2xl';
/** The breakpoint at which to apply the modifier */
breakpoint?: 'sm' | 'md' | 'lg' | 'xl' | '2xl';
}... /** Array of objects representing the various modifiers to apply to tabs at various breakpoints */
breakpointMods?: TabBreakpointMod[];... ... <Tabs
activeKey={activeTabKey}
onSelect={this.handleTabClick}
breakpointMods={[
{ breakpoint: 'md', modifier: 'insetSm' },
{ breakpoint: 'lg', modifier: 'insetLg' },
{ breakpoint: 'xl', modifier: 'inset_2xl' }
]}
isBox={isBox}>Let me know what you think!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. I see how this allows for a more flexible interface. But, I do we think if we make this change we should probably go back and update Drawer for consistency.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I think Drawer should use the same API and we should document it in the breaking change release notes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drawer is still beta. Do you mind opening the drawer issue. I will update the Tabs API.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened #4238 |
||
| /** Array of objects representing the various modifiers to apply to tabs at various breakpoints */ | ||
| breakpointMods?: TabsBreakpointMod[]; | ||
| } | ||
|
|
||
| export interface TabsState { | ||
| interface TabsState { | ||
| showScrollButtons: boolean; | ||
| disableLeftScrollButton: boolean; | ||
| disableRightScrollButton: boolean; | ||
| shownKeys: (string | number)[]; | ||
| } | ||
|
|
||
|
|
@@ -53,21 +70,26 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| constructor(props: TabsProps & OUIAProps) { | ||
| super(props); | ||
| this.state = { | ||
| showScrollButtons: false, | ||
| disableLeftScrollButton: false, | ||
| disableRightScrollButton: false, | ||
| shownKeys: [this.props.activeKey] // only for mountOnEnter case | ||
| }; | ||
| } | ||
|
|
||
| static defaultProps: PickOptional<TabsProps> = { | ||
| className: '', | ||
| activeKey: 0, | ||
| onSelect: () => undefined as any, | ||
| isFilled: false, | ||
| isSecondary: false, | ||
| isVertical: false, | ||
| isBox: false, | ||
| leftScrollAriaLabel: 'Scroll left', | ||
| rightScrollAriaLabel: 'Scroll right', | ||
| variant: TabsVariant.div, | ||
| component: TabsComponent.div, | ||
| mountOnEnter: false, | ||
| unmountOnExit: false | ||
| unmountOnExit: false, | ||
| breakpointMods: [] as TabsBreakpointMod[] | ||
| }; | ||
|
|
||
| handleTabClick( | ||
|
|
@@ -94,6 +116,28 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| } | ||
| } | ||
|
|
||
| handleScrollButtons = () => { | ||
| if (this.tabList.current && !this.props.isVertical) { | ||
| const container = this.tabList.current; | ||
| // get first element and check if it is in view | ||
| const overflowOnLeft = !isElementInView(container, container.firstChild as HTMLElement, false); | ||
|
|
||
| // get last element and check if it is in view | ||
| const overflowOnRight = !isElementInView(container, container.lastChild as HTMLElement, false); | ||
|
|
||
| const showScrollButtons = overflowOnLeft || overflowOnRight; | ||
|
|
||
| const disableLeftScrollButton = !overflowOnLeft; | ||
| const disableRightScrollButton = !overflowOnRight; | ||
|
|
||
| this.setState({ | ||
| showScrollButtons, | ||
| disableLeftScrollButton, | ||
| disableRightScrollButton | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| scrollLeft = () => { | ||
| // find first Element that is fully in view on the left, then scroll to the element before it | ||
| if (this.tabList.current) { | ||
|
|
@@ -133,6 +177,30 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| } | ||
| }; | ||
|
|
||
| // Format the inset prop string by capitalizing the first letter. If the string prop is '2xl' append '_' to beginning of the string so we can key the correct modifier. | ||
| formatInsetString = (s: string) => (s === '2xl' ? '_2xl' : capitalize(s)); | ||
|
|
||
| // Format the breakpoints array | ||
| formatTabBreakpointMods = (breakpointMods: TabsBreakpointMod[]) => | ||
| breakpointMods.map( | ||
| mod => | ||
| styles.modifiers[`${mod.modifier}On${this.formatInsetString(mod.breakpoint)}` as keyof typeof styles.modifiers] | ||
| ); | ||
|
|
||
| componentDidMount() { | ||
| if (!this.props.isVertical) { | ||
| window.addEventListener('resize', this.handleScrollButtons, false); | ||
| // call the handle resize function to check if scroll buttons should be shown | ||
| this.handleScrollButtons(); | ||
| } | ||
| } | ||
|
|
||
| componentWillUnmount() { | ||
| if (!this.props.isVertical) { | ||
| window.removeEventListener('resize', this.handleScrollButtons, false); | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
| const { | ||
| className, | ||
|
|
@@ -141,19 +209,23 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| id, | ||
| isFilled, | ||
| isSecondary, | ||
| isVertical, | ||
| isBox, | ||
| leftScrollAriaLabel, | ||
| rightScrollAriaLabel, | ||
| 'aria-label': ariaLabel, | ||
| variant, | ||
| component, | ||
| ouiaId, | ||
| mountOnEnter, | ||
| unmountOnExit, | ||
| inset, | ||
| breakpointMods, | ||
| ...props | ||
| } = this.props; | ||
| const { shownKeys } = this.state; | ||
| const { showScrollButtons, disableLeftScrollButton, disableRightScrollButton, shownKeys } = this.state; | ||
|
|
||
| const uniqueId = id || getUniqueId(); | ||
| const Component: any = variant === TabsVariant.nav ? 'nav' : 'div'; | ||
| const Component: any = component === TabsComponent.nav ? 'nav' : 'div'; | ||
|
|
||
| return ( | ||
| <React.Fragment> | ||
|
|
@@ -163,6 +235,11 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| styles.tabs, | ||
| isFilled && styles.modifiers.fill, | ||
| isSecondary && styles.modifiers.secondary, | ||
| isVertical && styles.modifiers.vertical, | ||
| isBox && styles.modifiers.box, | ||
| showScrollButtons && !isVertical && styles.modifiers.scrollable, | ||
| this.formatTabBreakpointMods(breakpointMods), | ||
| inset && styles.modifiers[`inset${this.formatInsetString(inset)}` as keyof typeof styles.modifiers], | ||
| className | ||
| )} | ||
| {...getOUIAProps('Tabs', ouiaId)} | ||
|
|
@@ -173,10 +250,12 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| className={css(styles.tabsScrollButton, isSecondary && buttonStyles.modifiers.secondary)} | ||
| aria-label={leftScrollAriaLabel} | ||
| onClick={this.scrollLeft} | ||
| disabled={disableLeftScrollButton} | ||
| aria-hidden={disableLeftScrollButton} | ||
| > | ||
| <AngleLeftIcon /> | ||
| </button> | ||
| <ul className={css(styles.tabsList)} ref={this.tabList}> | ||
| <ul className={css(styles.tabsList)} ref={this.tabList} onScroll={this.handleScrollButtons}> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar question here, should we only call the function if |
||
| {React.Children.map(children, (child: any, index) => { | ||
| const { title, eventKey, tabContentRef, id: childId, tabContentId, ...rest } = child.props; | ||
| return ( | ||
|
|
@@ -204,6 +283,8 @@ export class Tabs extends React.Component<TabsProps & OUIAProps, TabsState> { | |
| className={css(styles.tabsScrollButton, isSecondary && buttonStyles.modifiers.secondary)} | ||
| aria-label={rightScrollAriaLabel} | ||
| onClick={this.scrollRight} | ||
| disabled={disableRightScrollButton} | ||
| aria-hidden={disableRightScrollButton} | ||
| > | ||
| <AngleRightIcon /> | ||
| </button> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
It appears that
TabTitleTextis required for proper styling. Should the type reflect that?Or, at least note that in the comments?
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.
Can you also add that to your PR description? Looks like the components to use are not mentioned