Skip to content
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.

Sign up for GitHub

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

feat(tabs): add secondary border-bottom variation, update demos #7311

Merged
merged 3 commits into from
May 5, 2022
Merged

feat(tabs): add secondary border-bottom variation, update demos #7311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a26435f
feat(tabs): add secondary border-bottom variation, update demos
wise-king-sullyman Apr 28, 2022
f37f2f1
improve prop description
wise-king-sullyman May 4, 2022
5a69d30
sentence case test aria labels
wise-king-sullyman May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/react-core/src/components/Tabs/Tabs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ export interface TabsProps extends Omit<React.HTMLProps<HTMLElement | HTMLDivEle
isBox?: boolean;
/** Enables vertical tab styling */
isVertical?: boolean;
/** Enables no border bottom tab styling */
/** Enables border bottom tab styling on tabs. Defaults to true. To remove the bottom border, set this prop to false. */
hasBorderBottom?: boolean;
Copy link
Contributor

@jenny-s51 jenny-s51 May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hasBorderBottom?: boolean;
hasNoBorderBottom?: boolean;

Based on the prop description, I'm guessing you tested a hasNoBorderBottom prop, which is what we want based on the core docs: https://patternfly-pr-4774.surge.sh/components/tabs/

The (default, non-secondary) tabs have a bottom border by default, so it makes sense to have a prop that lets a consumer remove the bottom border.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah hasBorderBottom was originally hasNoBorderBottom in #7183 when I first PR'd it. @kmcfaul @nicolethoen I'd appreciate your thoughts on this suggestion. I'm a fan but since you both advocated for hasBorderBottom I'm hesitant to change it before getting your input.

Thank you for putting this much thought into it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still like hasBorderBottom over hasNoBorderBottom personally, where passing false adds the modifier css. I don't think it needs to match core's modifiers 1:1 in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, thank you for linking the comment thread @wise-king-sullyman and for sharing your input @kmcfaul !

Avoiding the double negative makes sense. If that's the case then we would want to keep both hasBorderBottom and hasSecondaryBorderBottom for the styling to work as expected. I've added a suggestion to update the prop description which would clarify the use case of hasBorderBottom a bit better - WDYT @wise-king-sullyman?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That suggestion looks great to me @jenny-s51, thank you for doing it!

/** Enables border bottom styling for secondary tabs */
hasSecondaryBorderBottom?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a new prop necessary? if the consumer specifies isSecondary and hasBorderBottom, could it do the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated that myself, but since hasBorderBottom defaults to true I worried that doing so would cause visual changes to applications that currently use secondary tabs which may not want the bottom border.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can update this with the next breaking change release? Assuming we want secondary tabs to also default to having a bottom border like default tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmcfaul I think that would be ideal.

/** Aria-label for the left scroll button */
leftScrollAriaLabel?: string;
/** Aria-label for the right scroll button */
Expand Down Expand Up @@ -292,6 +294,7 @@ export class Tabs extends React.Component<TabsProps, TabsState> {
isVertical,
isBox,
hasBorderBottom,
hasSecondaryBorderBottom,
leftScrollAriaLabel,
rightScrollAriaLabel,
'aria-label': ariaLabel,
Expand Down Expand Up @@ -361,6 +364,7 @@ export class Tabs extends React.Component<TabsProps, TabsState> {
showScrollButtons && !isVertical && styles.modifiers.scrollable,
usePageInsets && styles.modifiers.pageInsets,
!hasBorderBottom && styles.modifiers.noBorderBottom,
hasSecondaryBorderBottom && styles.modifiers.borderBottom,
Comment on lines 366 to +367
Copy link
Contributor

@mcoker mcoker May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this work to keep a single prop? Nope! I see the issue with it now.

Suggested change
!hasBorderBottom && styles.modifiers.noBorderBottom,
hasSecondaryBorderBottom && styles.modifiers.borderBottom,
!isSecondary && !hasBorderBottom && styles.modifiers.noBorderBottom,
isSecondary && hasBorderBottom && styles.modifiers.borderBottom,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would still cause breaking changes since hasBorderBottom defaults to true, so it would require consumers to add hasBorderBottom={false} if they want to keep their secondary tabs without a bottom border.

formatBreakpointMods(inset, styles),
variantStyle[variant],
className
Expand Down
41 changes: 40 additions & 1 deletion packages/react-core/src/components/Tabs/__tests__/Tabs.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';
import { Tabs } from '../Tabs';
import { Tab } from '../Tab';
import { TabTitleText } from '../TabTitleText';
Expand Down Expand Up @@ -350,3 +350,42 @@ test('should render tabs with no bottom border', () => {
expect(asFragment()).toMatchSnapshot();
});

test('should not render tabs with secondary border bottom when not passed hasSecondaryBorderBottom', () => {
render(
<Tabs id="noBottomBorderTabs" aria-label="Secondary bottom border">
<Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}>
Tab 1 section
</Tab>
<Tab id="tab2" eventKey={1} title={<TabTitleText>"Tab item 2"</TabTitleText>}>
Tab 2 section
</Tab>
<Tab id="tab3" eventKey={2} title={<TabTitleText>"Tab item 3"</TabTitleText>}>
Tab 3 section
</Tab>
</Tabs>
);

const tabsContainer = screen.queryByLabelText('Secondary bottom border');

expect(tabsContainer).not.toHaveClass('pf-m-border-bottom');
});

test('should render tabs with secondary border bottom when passed hasSecondaryBorderBottom', () => {
render(
<Tabs id="noBottomBorderTabs" aria-label="Secondary bottom border" hasSecondaryBorderBottom>
<Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}>
Tab 1 section
</Tab>
<Tab id="tab2" eventKey={1} title={<TabTitleText>"Tab item 2"</TabTitleText>}>
Tab 2 section
</Tab>
<Tab id="tab3" eventKey={2} title={<TabTitleText>"Tab item 3"</TabTitleText>}>
Tab 3 section
</Tab>
</Tabs>
);

const tabsContainer = screen.queryByLabelText('Secondary bottom border');

expect(tabsContainer).toHaveClass('pf-m-border-bottom');
});
110 changes: 53 additions & 57 deletions packages/react-core/src/demos/Tabs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,65 +378,60 @@ TabsOpenWithSecondaryTabsDemo = () => {
<Tab eventKey={4} title={<TabTitleText>Terminal</TabTitleText>} tabContentId={`tabContent${4}`} />
</Tabs>
</PageSection>
<PageSection isWidthLimited variant={PageSectionVariants.light}>
<Flex direction={{ default: 'column' }}>
<FlexItem>
<TabContent key={0} eventKey={0} id={`tabContent${0}`} activeKey={activeTabKey} hidden={0 !== activeTabKey}>
<TabContentBody>
<Tabs
isSecondary
activeKey={activeTabKeySecondary}
onSelect={handleTabClickSecondary}
inset={{ default: 'insetNone' }}
id="open-with-secondary-tabs-example-tabs-list-secondary"
>
<Tab
eventKey={10}
title={<TabTitleText>Pod information</TabTitleText>}
tabContentId={`tabContent${10}`}
/>
<Tab
eventKey={11}
title={<TabTitleText>Editable aspects</TabTitleText>}
tabContentId={`tabContent${11}`}
/>
</Tabs>
<TabContent
key={10}
eventKey={10}
id={`tabContent${10}`}
activeKey={activeTabKeySecondary}
hidden={10 !== activeTabKeySecondary}
>
<TabContentBody>{tabContent}</TabContentBody>
</TabContent>
<TabContent
key={11}
eventKey={11}
id={`tabContent${11}`}
activeKey={activeTabKeySecondary}
hidden={11 !== activeTabKeySecondary}
>
<TabContentBody>Editable aspects</TabContentBody>
</TabContent>
</TabContentBody>
</TabContent>
</FlexItem>
<FlexItem>
<TabContent key={1} eventKey={1} id={`tabContent${1}`} activeKey={activeTabKey} hidden={1 !== activeTabKey}>
<TabContentBody>YAML panel</TabContentBody>
</TabContent>
<TabContent key={2} eventKey={2} id={`tabContent${2}`} activeKey={activeTabKey} hidden={2 !== activeTabKey}>
<TabContentBody>Environment panel</TabContentBody>
</TabContent>
<TabContent key={3} eventKey={3} id={`tabContent${3}`} activeKey={activeTabKey} hidden={3 !== activeTabKey}>
<TabContentBody>Events panel</TabContentBody>
<PageSection isWidthLimited variant={PageSectionVariants.light} padding={{ default: 'noPadding' }}>
<TabContent key={0} eventKey={0} id={`tabContent${0}`} activeKey={activeTabKey} hidden={0 !== activeTabKey}>
<TabContentBody>
<Tabs
isSecondary
hasSecondaryBorderBottom
activeKey={activeTabKeySecondary}
onSelect={handleTabClickSecondary}
usePageInsets
id="open-with-secondary-tabs-example-tabs-list-secondary"
>
<Tab
eventKey={10}
title={<TabTitleText>Pod information</TabTitleText>}
tabContentId={`tabContent${10}`}
/>
<Tab
eventKey={11}
title={<TabTitleText>Editable aspects</TabTitleText>}
tabContentId={`tabContent${11}`}
/>
</Tabs>
<TabContent
key={10}
eventKey={10}
id={`tabContent${10}`}
activeKey={activeTabKeySecondary}
hidden={10 !== activeTabKeySecondary}
>
<TabContentBody hasPadding>{tabContent}</TabContentBody>
</TabContent>
<TabContent key={4} eventKey={4} id={`tabContent${4}`} activeKey={activeTabKey} hidden={4 !== activeTabKey}>
<TabContentBody>Terminal panel</TabContentBody>
<TabContent
key={11}
eventKey={11}
id={`tabContent${11}`}
activeKey={activeTabKeySecondary}
hidden={11 !== activeTabKeySecondary}
>
<TabContentBody>Editable aspects</TabContentBody>
</TabContent>
</FlexItem>
</Flex>
</TabContentBody>
</TabContent>
<TabContent key={1} eventKey={1} id={`tabContent${1}`} activeKey={activeTabKey} hidden={1 !== activeTabKey}>
<TabContentBody>YAML panel</TabContentBody>
</TabContent>
<TabContent key={2} eventKey={2} id={`tabContent${2}`} activeKey={activeTabKey} hidden={2 !== activeTabKey}>
<TabContentBody>Environment panel</TabContentBody>
</TabContent>
<TabContent key={3} eventKey={3} id={`tabContent${3}`} activeKey={activeTabKey} hidden={3 !== activeTabKey}>
<TabContentBody>Events panel</TabContentBody>
</TabContent>
<TabContent key={4} eventKey={4} id={`tabContent${4}`} activeKey={activeTabKey} hidden={4 !== activeTabKey}>
<TabContentBody>Terminal panel</TabContentBody>
</TabContent>
</PageSection>
</DashboardWrapper>
);
Expand Down Expand Up @@ -467,6 +462,7 @@ TabsOpenWithSecondaryTabsDemo = () => {

```js isFullscreen file="./examples/Tabs/ModalTabs.tsx"
```

### Gray tabs

```js isFullscreen file="./examples/Tabs/GrayTabs.tsx"
Expand Down