Toolbar - support for vertical alignment variations

[mcoker]

core PR: patternfly/patternfly#5201

[gitdallas]

looks like we are still waiting for the core pr to go in.

after that happens, i think we need to give the users the ability to add...

.pf-m-align-items-center to ToolbarContent and ToolbarGroup
.pf-m-align-self-center to ToolbarGroup and ToolbarItem
.pf-m-align-self-baseline to ToolbarGroup and ToolbarItem

What should we call these and what should they be under though? Would they be in addition to (outside of) the alignment prop like on ToolbarContent?

maybe..

ToolbarContent would add the verticalAlignItemsCenter:boolean
ToolbarGroup would add verticalAlignItemsCenter:boolean, verticalAlign: 'center' | 'baseline'
ToolbarItem would add verticalAlign: 'center' | 'baseline'
?

@mcoker @mattnolting

i may also need a demo or example of exactly what is happening with this, screenshots in core story/pr seem to show cramming some text in under a dropdown menu?

[mcoker]

@gitdallas good question - looks like alignment on <ToolbarContent> (.pf-c-toolbar__content) can be removed, I only see support for it on .pf-c-toobar_item and .pf-c-toolbar__group. https://github.com/patternfly/patternfly/blob/86be0974e7544bcc9d0119f85720277dff8290f0/src/patternfly/components/Toolbar/toolbar.scss#L477-L495

If possible, I think it would be good to match the property name where possible (align-items, or an alignItems prop in this case) so as not to run into naming conflicts in the future. Though sometimes that can be unintuitive, since this vertical alignment of the children would be align-items and horizontal alignment (if we added that) would be justify-content, which aren't as clear as verticalAlignItems and horizontalAlignItems. And just to abstract things a bit further, if there were a vertical toolbar, the align-items property in CSS would refer to horizontal alignment of the children, and justify-content would refer to vertical alignment.

The toolbar is mostly just an opinionated flex layout. If possible, I think matching the flex layout property names here (and for the existing alignment) prop would be my suggestion.

• alignment would change to align. This applies to a child of a flex layout, and aligns that child (and any adjacent children on the other side of it) to one side of the main axis.

  • patternfly-react/packages/react-core/src/layouts/Flex/Flex.tsx

    Line 249 in bf9cf83

    align?: {

• use alignItems for this property, which applies to a flex parent and vertically aligns its children

  • patternfly-react/packages/react-core/src/layouts/Flex/Flex.tsx

    Line 176 in bf9cf83

    alignItems?: {

I also think moving in the direction of "start" and "end" instead of "left" and "right" will move us in the direction of RTL language support (see logical properties/values), and the flex property values already do that

[gitdallas]

@mcoker
trying to wrap my head around this to translate it into what props actually need to be added and the resulting styles.

patternfly/patternfly#5201 says it added 3 classes, so i was assuming i just needed to have props that add those 3 classes in the right places.

This is where I'm at now:

ToolbarContent:

• remove alignment
• add align (what classes would this add? or just work the same way alignment used to?)
• add alignItems (boolean to add .pf-m-align-items-center class as it seems to be the only align-items class in core pr)

ToolbarGroup

• remove alignment and add align here as well?
• add alignItems (boolean to add .pf-m-align-items-center)
• add alignSelf: 'center' | 'baseline' (to add either .pf-m-align-self-center or .pf-m-align-self-baseline?)

ToolbarItem

• remove alignment and add align here as well?
• add alignSelf: 'center' | 'baseline' (to add either .pf-m-align-self-center or .pf-m-align-self-baseline?)

for alignSelf... would default just leave things the way they are and not add any class?

[mcoker]

This is what I was thinking. Since none of these prop values are required, "default" is just an option to choose from that uses whatever the default is from core - no class in this case.

ToolbarGroup

• alignItems: 'center' | 'baseline' | 'default'
• alignSelf: 'center' | 'baseline' | 'default'
• align: 'right' | 'left' | 'default'

ToolbarItem

• alignSelf: 'center' | 'baseline' | 'default'
• align: 'right' | 'left' | 'default'

ToolbarContent

• alignItems: 'center' | 'default'
  • Note, this will go on toolbarContentSection

    patternfly-react/packages/react-core/src/components/Toolbar/ToolbarContent.tsx

    Line 113 in 98f8a72

    <div className={css(styles.toolbarContentSection)}>{children}</div>

I do think we could update align to be start/end instead of left/right, but it would be a bit inconsistent since our classnames are currently left/right, and other components use left/right, too. I think to support RTL with logical properties, the change to start/end could be a separate effort.

cc @mattnolting wdyt - is this inline with what you'd expect to see?