feat(Toolbar): allow multiple toggle groups

[kmcfaul]

What: Closes #7590

First pass on multiple toggle groups.

Multiple toggle groups (and filter chip groups in different toggle groups) will work separately but currently share a container portal (as in toggling will add/subtract from the same container). I'm experimenting with replacing portal contents, or using separate portals. Unless a single portal will work, WDYT? @nicolethoen

Overall the current approach requires manual management of the expanded state, and requires passing that expanded state logic to multiple components (ToolbarContent, ToolbarToggleGroup, ToolbarFilter) to bypass internal logic which only supports a single toggle group. There is a temp example up on the Toolbar doc page that I'll remove before merging which showcases it.

[patternfly-build]

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

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

[nicolethoen]

When I had been experimenting with this once upon a time, I tried doing it with separate portals. I think it's generally promising, even though it does require manipulation of more pieces.

When playing with your test example, I do notice that when I expand the filter group at a small screen width, then begin resizing the window to make it wide, the styles are not adjusting like I expect them to. I'm not sure if thats a side effect of the approach.

[kmcfaul]

This second pass exposes ToolbarExpandableContent and uses multiple expandable groups to prevent new elements from being added/subtracted to the same popup. I've cleaned up the example to be clearer and added some comments about what is enabling the multiple toggle groups as well.

In summary, you need:

• separate expanded state handling and tracking for each toggle group (passed to each ToolbarToggleGroup)
• a ToolbarExpandableContent for each toggle group, collectively passed to ToolbarContent in a fragment
• refs for each expandable content (passed to each ToolbarToggleGroup) and expandable content chip group (passed to each ToolbarFilter). If the toggle groups don't contain a filter with chips then the second ref shouldn't be needed
• a resize observer to collapse toggle groups so they reformat properly as the window grows. I'm not sure yet why resizing while the popup is expanded results in odd formatting

Exposing ToolbarExpandableContent let chip groups be separated out as each group collapses, and allows the customization of the clear filter props per toggle group. However, the internal chip group container that is displayed for the full toolbar is still displaying all chip groups until its own collapse breakpoint is hit. Subtracting each chip group as each toggle group collapses would require more tinkering if we want to add that but should be doable (might have to pass the expanded state through to ToolbarFilter is my initial thought).

The other slight issue is there is a grey area between ~1000px and ~1200px where a toggle group does not expand as a popup menu but instead inline, but I think that's unrelated to the changes here. @mattnolting

[kmcfaul]

@nicolethoen How does this look for a solution with the new changes? If the approach looks good lmk and I'll take it out of draft.

My other open question for all is whether we want to include the example I've created in the Toolbar docs.

[nicolethoen]

I wonder if instead of asking the consumer to define their own expandable content, if we can simply create it when they need it rather than having one preexisting whose ref we pass around for using in createPortals.

In that case we could take the children passed to the ToolbarToggleGroup and put them in an ExpandableContent component and place that ExpandableContent in a Popper? WDYT?

[kmcfaul]

I had experimented with putting the ToolbarExpandableContent inside the ToolbarToggleGroup but the layout/pop-out behavior wasn't working with the createPortal properly. That's why I went with the extra property in ToolbarContent so the DOM structure doesn't change and the expandable content looks the same. I can try with a Popper and see if it behaves better.

A user would still need to pass the chip refs around even with an internal popper, because chips are controlled in the ToolbarFilter. The other potential drawback would be that the additional properties controlling the chip deletion that are passed to the expandable content would need to be piped through ToolbarToggleGroup, which means a user is defining the expandable content manually still.

[nicolethoen]

idk if it helps - i have a SUPER old branch where I tinkered with this during a break away sprint. I think I removed the need for create portal.

I just made a draft PR so you can easily see the diff: #9349

[kmcfaul]

Take 3 with poppers.

The caveat is that the spacing is now different, because there is no top padding on the expandable content, and popper is appending directly after the toolbar (while the createPortal method looked like it was in line with a grid gap? Shown in all toggle examples but most noticeable in the stacked example as the divider line is being lost now). The weird zone between 1000-1200px also looks more squished as the pop up is losing padding.

The user doesn't have to manually create the ToolbarExpandableContent component with this setup, though they would still have to pipe a lot of those props through ToolbarToggleGroup.

[kmcfaul]

@nicolethoen

LMK what you think between the popper approach and the previous manual expandable content one. The manual expandable contents may require a bit more user definition, but the current structure of the toolbar is better preserved than when using popper.

[nicolethoen]

I like it better with less that the consumer has to wire up.
Is there a way place the expandable content div inside the popper div? would that solve the spacing issue?

[kmcfaul]

The entire expandableContent is already given to Popper, or do you mean a different div?

If you mean the div where the popper is appending to, that is how the toggle group is appearing below the toolbar line (otherwise the expandable content would be inline with the trigger filter icon).

[nicolethoen]

I played with it a bit and was trying to figure out why the padding in the expandable content was being removd/overwritten.
If it was more like

<div /** popper div */>
  <div className="pf-c-toolbar__expandable-content">
    ...
  </div>
<div>

rather than

<div /** popper div */ className="pf-c-toolbar__expandable-content">
    ...
<div>

[kmcfaul]

@nicolethoen I tested wrapping the expandableContent in an additional div to separate out the styling from popper, but it appears something in the two sets of styles is still in conflict. The positioning of the popper also becomes inconsistent (sometimes it will attach to the top of the page instead and only a reload reverts that position).

Screen.Recording.2023-08-07.at.11.13.49.AM.mov

[mattnolting]

@kmcfaul This is looking great!

Looks like we need to offset popper by the gap value

[kmcfaul]

@mattnolting Would the gap class be a pf-u- class or something else?

[mattnolting]

@mattnolting Would the gap class be a pf-u- class or something else?

@kmcfaul looks like you could either remove the inline styles popper is applying (which looks easiest and would avoid conflicting styles) or you could follow the core model

[mattnolting]

Looks like pf-m-chip-container isn't being applied to pf-v-5-c-toolbar__content, which throws alignment off a bit

[mattnolting]

LPTM! Fantastic work, @kmcfaul, love this enhancement 👍

[nicolethoen]

It's looking good! Do you see this behavior in the codesandbox?
It looks like the other controls are being hidden when I toggle open the toggle group?

[kmcfaul]

@nicolethoen I'm not sure what's going on with the codesandbox on the preview - it looks like its not actually using the PR code for it. On the surge, you'll see that the expandable-content section doesn't exist when the toggle group is closed (whereas previously it would always exist, and still does on the codesandbox).

[wise-king-sullyman]

Looks great! I do think an integration test for this bug would be nice, as would unit tests for the new props, but I won't block over it.

[andrew-ronaldson]

LGTM, Thanks!