patternfly icon indicating copy to clipboard operation
patternfly copied to clipboard

Published 20 hours ago • verified publisher icon patternfly

Toolbar component review

Open mcoker opened this issue 1 year ago • 2 comments
trafficstars

Looking at some changes in https://github.com/patternfly/patternfly/pull/6057 and issues that have resulted, the toolbar styles should be reviewed as a whole for feature parity with react and cleaned up wherever possible. For example, it was determined in https://github.com/patternfly/patternfly-react/pull/10418#issuecomment-2127444738 that we didn't need the .pf-m-margin-inline-[start/end]-[none/xs/sm/md/lg/xl/2xl/3xl/4xl]-on-[sm/md/lg/xl/2xl] modifiers that were added. A quick test of pulling that out slims the toolbar stylesheet down from 2921 lines and 128k in size to 2273 lines and 96k. For comparison, the v5 toolbar stylesheet is around 50k, and the v6 stylesheet around 130k.

--

Starting with the modifiers that generate a lot of CSS, review each of them for the following

  • Is the feature required? For example, it's required structurally, it's an approved user request or enhancement for v6, it's a pre-existing feature (that was in core and react in v5) that doesn't need to change.
    • Remove what is clearly not needed, make a note of anything else if there are questions.
  • Is it in react? Whether it's new for v6 or was in v5, if there is no react support for a feature, make a note of it and let's review and see if it's something we need to keep or can be removed.
  • Make sure the feature is documented (there are appropriate examples and classes/what-not are in the docs table), and that the documentation is accurate.

Here is a rough list of the modifiers in question, but verify this in case there are any I missed:

Hidden/visible - supported on content, content-section, group, item, group.pf-m-label-group-container, group.pf-m-label-group

Why is it on .#{$toolbar}__group 3 times?

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L67-L72

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L77-L79

Width/min-width vars

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L189-L191

Alignment on item and group

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L305-L307

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L309-L312

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L314-L316

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L318-L320

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L323-L325

Alignment on group, item, content, content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L355-L357

Insets on toolbar, content, and content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L330-L339

Wrapping on group, item, content, content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L346-L348

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L350-L352

Gaps on group, item, content, content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L361-L364

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L369-L371

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L373-L375

Margins on group, item, content, content-section - remove these

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L377-L383

Show/hide on group.pf-m-toggle-group

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L389-L399

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L402-L411

mcoker avatar May 28 '24 18:05 mcoker

Hidden/visible - supported on content, content-section, group, item, group.pf-m-label-group-container, group.pf-m-label-group

Why is it on .#{$toolbar}__group 3 times?

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L67-L72

Hidden/visible update:

Duplicate selectors should be removed

~~.#{$toolbar}__group.pf-m-label-group-container~~ ~~.#{$toolbar}__group.pf-m-label-group~~



https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L77-L79

Toolbar content display update:

Retain this. .pf-v6-c-toolbar__content houses pf-v6-c-toolbar__content-sections and could be used to hide/display whole sections of toolbar, especially in mobile presentation.



Width/min-width vars

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L189-L191

Min width update:

Due to the complexity and size of this feature, it can be simplified to width/min-width vars without responsive extensions.



Alignment on item and group

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L305-L307

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L309-L312

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L314-L316

Alignment update:

Alignment modifiers work as expected, we should retain them.

Screenshot 2024-06-21 at 12 04 31 PM Screenshot 2024-06-21 at 11 47 45 AM



https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L318-L320

Flex grow update:

Flex grow modifier work as expected, we should retain it.

Screenshot 2024-06-21 at 12 06 16 PM



https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L323-L325

Alignment on group, item, content, content-section

Alignment update:

Alignment should be isolated to group, item, and content-sections



https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L355-L357

Insets on toolbar, content, and content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L330-L339

Inset update:

Insets can be isolated to content-section as gap is adjustable for content-section, group, and item.



Wrapping on group, item, content, content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L346-L348

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L350-L352

Wrapping update:

Until we have a better approach to handling overflow, this is the simplest alternative as it allows overflowing groups and items to wrap. However, it should be isolated to content-section, group, and item.



Gaps on group, item, content, content-section

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L361-L364

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L369-L371

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L373-L375

Gap update:

Gap for content should remain consistent. Gap for group, item, content and content-section should remain adjustable.



Margins on group, item, content, content-section - remove these

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L377-L383

Margin update:

Remove these.



Show/hide on group.pf-m-toggle-group

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L389-L399

https://github.com/patternfly/patternfly/blob/378c9b11d56555ac5754a79266c1dcd03717edaa/src/patternfly/components/Toolbar/toolbar.scss#L402-L411

Show/hide update:

Retain show/hide for resize observer functionality when available. This feature supports selector updates when available space is inadequate, without DOM manipulation. However, we might discuss updating show/hide to hidden/visible.



mattnolting avatar Jun 21 '24 16:06 mattnolting

:tada: This issue has been resolved in version 6.0.0-alpha.168 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

patternfly-build avatar Jun 21 '24 18:06 patternfly-build

closed via #6838

mattnolting avatar Jul 11 '24 12:07 mattnolting