[components] Add panel toggle icon overrides

[danieliser]

What?

This adds support for customizing the PanelBody toggle icons beyond just chevronUp & chevronDown.

Why?

This allows replacing the up & down icons with something else for better context.

Without this a developer is required to duplicate 4+ files to remake the needed components. This is primarily due to usage of internal functions that are not exposed import { useControlledState, useUpdateEffect } from '../utils';

How?

• Adds new iconOpen & iconClosed props with defaults set to current chevron* icons. I had considered iconUp & iconDown but decided these were more explicit. They could be even more specific by changing them to toggleIconUp or similar. Open to suggestion.
• Updated readme to reflect new props.
• Updated stories with new example.

Testing Instructions

1. Create a Panel & PanelBody.
2. Add iconOpen and/or iconClosed props to PanelBody.
3. Render/run and view new ability to customize them.

Screenshots or screencast

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @danieliser! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[amustaque97]

Thank you for working on this. I tested it locally and it is working fine. 👍🏻

[danieliser]

@amustaque97 - Not a problem. Scratched an itch. Looking forward to doing so more often 💯

[danieliser]

I didn't add changelog note, wasn't sure if that was something I do as part of the PR or done when it was merged.

Happy to add one now if I should have done so.

[amustaque97]

I didn't add changelog note, wasn't sure if that was something I do as part of the PR or done when it was merged.

Happy to add one now if I should have done so.

Yeah, feel free to add changelog now. 🙂

[ciampo]

Hey @danieliser , thank you for working on this!

I left a few inline comments.

As @amustaque97 said, please do add an entry to the CHANGELOG in this PR

---

It would be also great if these component's stories were converted from using knobs to using controls — although this could be totally done in a follow-up PR . Would you be up for that?

[ciampo]

These props are not Strings — let's update the description as follow:

Suggested change

	An icon to be shown as the toggle button within the `PanelBody` title when open.
	- Type: `String`
	- Required: No
	An icon to be shown as the toggle button within the `PanelBody` title, in its expanded state. See [the `@wordpress/icons` package](https://github.com/WordPress/gutenberg/blob/trunk/packages/icons/README.md) for a list of available icons.
	- Type: `React.ReactNode`
	- Required: No
	- Default: `chevronUp` icon from `@wordpress/icons`

[danieliser]

@ciampo - Sorry, wasn't sure what type they should be, I copied from the lines above documenting the existing icon prop which also said string.

Should I update the original icon prop documentation to match?

[ciampo]

Should I update the original icon prop documentation to match?

Yes please. I believe the icon prop being a String is a leftover from earlier times when dashicons were the main way to provide icons.

[ciampo]

We should either:

• use a select control to give users a few icons to choose from
• use boolean controls, but call them customIconExpanded and customIconCollapsed

[mirka]

Thank you for another great contribution!

As for the naming and API:

1. I feel strongly that we should use the word "disclosure" in the prop name (e.g. disclosureIcon). An unqualified icon is a little too vague here since we already have an icon prop, and the term "disclosure" is in line with web standards.

2. What do we think about just adding one prop (e.g. disclosureIcon), and have the consumer switch their own icon states via onToggle?

   // Not sure if this works, just writing off the top of my head
   const [ isOpen, setIsOpen ] = useState();
   return <PanelBody onToggle={ setIsOpen } disclosureIcon= { isOpen ? Foo : Bar } />

   I don't feel super strongly about this point, but I wanted to suggest it because this particular customization falls outside of established Gutenberg design patterns, so I'm hesitant to add two extra props just for this relatively niche use case.

[danieliser]

@mirka I'm open to consensus, but don't have an experienced enough opinion within this project to weigh in. Will see what others say.

My only 2 cents would be that I would personally have no clue as a developer using this component what disclosureIcon would be referencing. I've never seen this particular web standard I'm guessing.

[ciampo]

I feel strongly that we should use the word "disclosure" in the prop name. An unqualified icon is a little too vague here since we already have an icon prop

Given that @danieliser is proposing to add two props, at the moment named iconExpanded and iconCollapsed, what do you mean when you refer to "an unqualified icon" ?

What do we think about just adding one prop (e.g. disclosureIcon), and have the consumer switch their own icon states via onToggle? [...] I don't feel super strongly about this point, but I wanted to suggest it because this particular customization falls outside of established Gutenberg design patterns, so I'm hesitant to add two extra props just for this relatively niche use case.

How would this approach work when the component is uncontrolled?

---

Apart from that, for the newly added prop(s), should their name start or end with icon ? (e.g. iconCollapsed vs collapsedIcon)?

[mirka]

My only 2 cents would be that I would personally have no clue as a developer using this component what disclosureIcon would be referencing. I've never seen this particular web standard I'm guessing.

Fair point! That is something that we can mitigate via docs (readme and Storybook) though. In general we'll want to keep our terminology in line with the underlying web standards (e.g. ARIA disclosure widget, <detail>) so we don't make up our own local terms or clash with existing concepts.

I feel strongly that we should use the word "disclosure" in the prop name. An unqualified icon is a little too vague here since we already have an icon prop

Given that @danieliser is proposing to add two props, at the moment named iconExpanded and iconCollapsed, what do you mean when you refer to "an unqualified icon" ?

Yeah that was unclear, sorry. Unqualified in the sense that expanded/collapsed are states of icon. If you only have two words "icon + collapsed", you are describing the state of icon. It would be less vague if we qualified what icon we're talking about (e.g. collapsedDisclosureIcon).

How would this approach work when the component is uncontrolled?

From my skimming of the code, this component only works uncontrolled. Hence the need to hook into onToggle to get the current state.

Apart from that, for the newly added prop(s), should their name start or end with icon ? (e.g. iconCollapsed vs collapsedIcon)?

My rule of thumb for naming things like this is "Can it be mistaken for a boolean?". So my vote goes to "ends with icon" 🙂

[mirka]

I'll also note that, if we were to design this component API from scratch today, I think we should be using the prefix/suffix pattern (like with InputControl) and not even mention "icon" or "disclosure" 😄 This is another reason why I slightly prefer not to introduce states into prop names (collapseFoo) if possible. Because theoretically, someone should be able to place their disclosure icon in the prefix slot instead of the suffix slot, and that would require yet another pair of collapse/expand props!

[ciampo]

I think that @mirka 's points here are definitely relevant — I'm in favour of applying her suggestion and, therefore, switch to one prop called disclosureIcon.

We will need to update docs, Storybook and CHANGELOG accordingly.

From my skimming of the code, this component only works uncontrolled

From a quick look, I thought PanelBody could be controlled via the opened prop (although this doesn't really affect the decision of switching to having one disclosureIcon prop)

[mirka]

From a quick look, I thought PanelBody could be controlled via the opened prop

Ah you're right, TIL useControlledState. Hopefully we can improve the docs around this in the near future

[danieliser]

I had considered handling it in a more programattic and single interface way where you could pass an icon, string or callback and the callback would get isExpanded as an argument.

In that way you could theoretically also forward current toggle state to the icon prop callback function.

Something like

<PanelBody disclosureIcon={ ( isOpened ) => isOpened ? chevronUp : chevronDown }>

And for rendering something along the lines of

const toggleIcon = typeof disclosureIcon === 'function' 
        ? disclosureIcon( isOpened )
        : ( isOpened
                ? chevronUp
                : chevronDown );

<Icon icon={ toggleIcon  } />

One thing of note, we changed icon labeling from open/closed, to expanded/collapsed, but all the internal logic is labeled open/closed. Just something to consider.

[ciampo]

My preference would be to follow @mirka 's suggestion, and have disclosureIcon accepting an SVG icon or a string.

I wouldn't add a callback, simply because the consumer of PanelBody would be able to know anyway if the component is in its expanded/opened or collapsed/closed state, both in controlled and uncontrolled modes.

we changed icon labeling from open/closed, to expanded/collapsed, but all the internal logic is labeled open/closed. Just something to consider.

There seems to be a lack of coherence in the way the docs and the code have been written: in some cases, it refer to the terms "expand" and "collapse" (example), but in others, it refers to "opened" and "closed" (like for the opened prop and the internal logic).

Anyway, if we introduce just a new disclosureIcon prop, we should avoid this altogether

[mtias]

May I ask for more examples of how this would be customized and for what reasons? Expandable panels have some assumed mechanics and the design might fluctuate assuming arrows (for example, relocating them on the left for ToolsPanels).

[danieliser]

@mtias - I can, though mine goes a little beyond what is proposed here.

Our version has a dropdown in the header when open, but the idea was similar and had this component supported what I was trying to do before I wrote a custom version I'd have gone a different route.

In our case showing a panel also enables features on the block immediately, it the panel being open is a setting in itself toggling on more advanced stuff.