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

[wise-king-sullyman]

What: Closes #7259

Additional issues:

Convenience link to the updated demo:
https://patternfly-react-pr-7311.surge.sh/components/tabs/react-demos/open-tabs-with-secondary-tabs/

[patternfly-build]

Preview: https://patternfly-react-pr-7311.surge.sh

A11y report: https://patternfly-react-pr-7311-a11y.surge.sh

[nicolethoen]

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

[wise-king-sullyman]

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.

[kmcfaul]

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.

[wise-king-sullyman]

@kmcfaul I think that would be ideal.

[mcoker]

LGTM! Just a couple of comments.

[mcoker]

nit - do we care about sentence case in these tests?

Suggested change

	<Tabs id="noBottomBorderTabs" aria-label="secondary bottom border">
	<Tabs id="noBottomBorderTabs" aria-label="Secondary bottom border">

[wise-king-sullyman]

It isn't consumer or user facing so it shouldn't make a big difference IMO. I'm not against making these sentence case though.

[mcoker]

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,

[wise-king-sullyman]

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.

[jenny-s51]

These changes are looking good @wise-king-sullyman! I did a lot of investigating here and have suggestions for changes that will help us get this merged without breaking anything! These changes should ease the breaking change updates down the road as well. Here's what we'd have to do:

1. Add a hasNoBorderBottom prop. Tabs (.pf-c-tabs) already have a bottom border by default, so instead of hasBorderBottom we'd want hasNoBorderBottom which will apply the CSS class .pf-m-no-border-bottom to the tabs.
2. Set hasSecondaryBorderBottom to false under defaultProps. Based on the core docs isSecondary i.e. (pf-m-secondary) styles the tabs with no border. So when a user sets hasSecondaryBorderBottom it should add the .pf-m-border-bottom to the secondary variant of Tabs.

The stuff we'd have left to do in a breaking change release would be to either:

1. Delete the hasBorderBottom prop completely (functionality is replaced by hasNoBorderBottom) OR
2. Rename hasSecondaryBorderBottom to hasBorderBottom and it would serve the same purpose -- based on the core docs the .pf-m-border-bottom class is only applied to secondary tabs anyway. Though leaving it as hasSecondaryBorderBottom would likely make more sense to a consumer as it tells them directly what Tabs variant that prop should be used with.

WDYT @wise-king-sullyman ? @mcoker please comment if you have any thoughts - I think I've interpreted the core side correctly 😄

[jenny-s51]

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.

[wise-king-sullyman]

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!

[kmcfaul]

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.

[jenny-s51]

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?

[wise-king-sullyman]

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

[mcoker]

LGTM!

[jenny-s51]

👍👍

[tlabaj]

LGTM