feat(divider): add support for switching orientation at various breakpoints

[jenny-s51]

What: Closes #7225

[patternfly-build]

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

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

[tlabaj]

I think we want to add a default here and deprecate the isVertical prop?
@mcoker, thoughts?

[mcoker]

@tlabaj yep, that would be inline with how we handle these breakpoint modifiers in other components.

[mcoker]

LGTM! Though I agree with @tlabaj, we should just use this instead of isVertical in the examples and plan to remove that prop in our next breaking change release.

[tlabaj]

@mcoker I will open an issue for the breaking change release.

[mcarrano]

Looks fine from my perspective

[jenny-s51]

Thank you @tlabaj @mcoker @mcarrano !

I've added default to the orientation prop, and updated the isVertical prop description to reflect that it is now deprecated.

I've also updated the vertical demos to use orientation.

[tlabaj]

@jenny-s51 I opened #7325 to remove deprecated prop when we have breaking change.

[tlabaj]

LGTM

[nicolethoen]

can we or should we set the default orientation alongside the other default props? Before, isVertical defaulted to false.

[jenny-s51]

Setting the default orientation shouldn't be necessary here since Divider defaults to horizontal! 👍

[mcoker]

LGTM - one nit about the docs, not sure if it's something we want to update in this PR or not.

Also looks like we use a vertical divider in other places - should we update those, too? Or anywhere else you can think we might have a vertical divider?

marvin:patternfly-react cmichael$ grep -ri '<Divider.*isVertical' packages/react-core/
packages/react-core//src/components/Divider/__tests__/Divider.test.tsx:      <Divider isVertical/>
packages/react-core//src/components/Divider/examples/DividerVerticalFlexInsetMedium.tsx:    <Divider isVertical inset={{ default: 'insetMd' }} />
packages/react-core//src/components/Divider/examples/DividerVerticalFlex.tsx:    <Divider isVertical />
packages/react-core//src/demos/CardDemos.md:              {content.length > 1 && index === 0 && <Divider key={`${index}_d`} isVertical />}
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />
packages/react-core//src/demos/CardDemos.md:                      <Divider className="pf-u-hidden-on-md" isVertical inset={{ default: 'insetSm' }} />

[mcoker]

Just a nit and outside the scope, but since we're copying this doc comment, is the bit about it needing to be in a flex layout necessary? I'm assuming so since the vertical divider doesn't really work on its own like the default/horiz variant. If we're going to leave it, technically the parent (or divider itself) just needs a height of some sort, or the parent needs to support align-items so that align-items: stretch on the divider works (this is why it works in flex, but that is supported in grid, too).

Here are a few examples - https://codepen.io/mcoker/pen/ZErzBeg

This is what the vertical divider core docs say

This modifier requires that the parent has an explicit or implicit height, or has a flex or grid based layout parent.

[jenny-s51]

Thank you for your helpful comment and clarification here @mcoker ! It looks like the vertical variant only renders in react when it's placed within a Flex layout -- the extra detail in the prop description lets the consumer know they'll need it.

Is setting the height and/or supporting align-items something that can be done in react, or would that need to be done in core? If agreeable with you, I'd be happy to open another issue so we can get this merged 🙂

[mcoker]

@jenny-s51 gotcha! I guess it's a little confusing to me since it can be used outside of a CSS flex layout, but mostly works in the <Flex> react layout.

[mcoker]

LGTM! Just one thing about a redundant vertical orientation specified for the sm breakpoint here -

patternfly-react/packages/react-core/src/components/Divider/examples/DividerOrientationVariousBreakpoints.tsx

Lines 7 to 15 in 91f32b9

	<Divider
	orientation={{
	default: 'vertical',
	sm: 'vertical',
	md: 'horizontal',
	lg: 'vertical',
	xl: 'horizontal'
	}}
	/>

If we're showing default-xl, IMO we may as well show 2xl in the example, too, and just switch between vertical/horizontal in each breakpoint, starting with vertical as you have in the example.

[mcoker]

If the default is vertical, specifying vertical at a small breakpoint is unnecessary. It will stay at whatever orientation specified at a breakpoint until the orientation changes at a larger breakpoint.

[jenny-s51]

good point, fixed 👍

[mcoker]

🤩

[jenny-s51]

Good call on the breakpoints demo, thank you @mcoker! I've added 2xl and updated the orientation prop to alternate between vertical and horizontal for each breakpt, starting with vertical as the default.

[mcoker]

🥳

[patternfly-build]

Your changes have been released in:

• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]

Thanks for your contribution! 🎉