Consume core Penta updates: Form controls, checkbox, radio

[adamviktora]

What: Closes #9931, closes #10055

Things done:

• introduced FormGroupLabelHelp component to make using labelIcon prop simple
• added some spaces next to label in FormGroup to look similar to core

Breaking changes done:

• renamed labelIcon prop in FormGroup to labelHelp
• renamed isLabelBeforeButton prop in Checkbox/Radio to labelPosition?: 'start' | 'end'

Todo:

• update examples of Checkbox to show isLabelWrapped and labelPosition="start" (issue: Checkbox - add examples of "Reversed" and "Label wraps" #10085)
• (add Form control examples in React too)

[patternfly-build]

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

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

[thatblindgeye]

I updated the labelIcon prop name to labelHelp since we're going to need to write a codemod just to mention that the markup has changed for that prop (we now wrap it in the labelHelp classname, whereas previously our examples showed that class being applied directly to a button).

• The examples to show isLabelWrapped and isLabelBeforeButton in Checkbox should be added in main since those updates were made in main.
• I don't believe we'll need to add FormControl examples since for React we have the Form Select, Text Input, and Text Area component pages
• I'd agree we could rename the isLabelBeforeButton prop in Checkbox and Radio, maybe to labelPosition?: 'start' | 'end', as it kind of goes inline with other props where we're using those values for. But that wouldn't be a priority and maybe for a post-v6 breaking change.

[kmcfaul]

I updated the labelIcon prop name to labelHelp since we're going to need to write a codemod just to mention that the markup has changed for that prop (we now wrap it in the labelHelp classname, whereas previously our examples showed that class being applied directly to a button).

labelHelp sounds a little incomplete to me but labelHelpButton is a little long (especially if FormGroupLabelHelpButton is updated to match) so I think this is fine. Just curious what others think.

• I'd agree we could rename the isLabelBeforeButton prop in Checkbox and Radio, maybe to labelPosition?: 'start' | 'end', as it kind of goes inline with other props where we're using those values for. But that wouldn't be a priority and maybe for a post-v6 breaking change.

A post-v6 breaking change would mean v7, so I think we should try to clean this up now versus later (depending on the remaining v6 work priority). I do like labelPosition for consistency.

[adamviktora]

I replaced the isLabelBeforeButton prop in Checkbox/Radio based on Katie's and Eric's suggestions with labelPosition?: 'start' | 'end'

Codemod issue followup: patternfly/pf-codemods#583

[mcoker]

One comment, but it's GTG as it is, too!

[mcoker]

I'll just mention that in core we use the <span> version of the button component here to help with line wrapping, though IIRC I don't know that it will make any sort of difference on the react side.

The reason for ^ is that some users want the help icon to wrap with the label text - or more specifically, they don't want the help icon to wrap on its own line (orphan), so it needs to wrap with at least one word of the label text. We can't get the label text to wrap with a <button>, but it will wrap with a <span>. However, another thing we can't get to wrap with text is an <svg> - which is how react-icons' icons are delivered. So even if we make the button a <span>, since react uses <svg>'s for the icons, it won't wrap with the text anyways.

I might suggest using the <span> anyways, if there are no real downsides to using that in favor of a <button>, on the off chance that we're able to figure out how to get an <svg> to wrap with text. That would also allow a user to pass a non-svg as the icon if we ever added that support - doesn't look like it's supported now.

[adamviktora]

@mcoker Good point Michael!

I remember I actually was applying component="span" and isInline props before to match the core implementation (which in combination leads to these attributes: type="button" role="button" tabindex="0" being added on the <span> element, which is exactly what is in core).

So I was wondering why it is not there now.

I was just about to commit your suggestion and then I realized why I did not apply the component="span" back then. The reason is accessibility of the Popover. Although the tabindex="0" makes the span focusable, the Popover cannot be opened on pressing the enter key. It works only if the element is a <button>.

span:
https://github.com/patternfly/patternfly-react/assets/84135613/a452671a-9523-4a64-993f-89029e81c17f

button:
https://github.com/patternfly/patternfly-react/assets/84135613/534086d0-7319-4ada-aebc-5c0664510a99

Seems like this issue is related to the implementation of the Popover, so I am taking a look at that now.

For now I would keep it as <button>, so the Popover is accessible, and once we figure that out, we can add the component="span" and isInline props to match core.

[mcoker]

@adamviktora nice, thanks for digging in and for the explanation! Sounds like a plan 👍👍

[tlabaj]

This is exported, do we have codemod issue for this?

[adamviktora]

Thanks for the catch! now we do: patternfly/pf-codemods#609

[tlabaj]

lgtm

[andrew-ronaldson]

There are a couple spacing things here but I think that falls more in a core followup issue so I am going to make that now.

[patternfly-build]

Your changes have been released in:

• @patternfly/react-code-editor@6.0.0-alpha.45
• @patternfly/react-core@6.0.0-alpha.45
• @patternfly/react-docs@7.0.0-alpha.47
• @patternfly/react-drag-drop@6.0.0-alpha.26
• @patternfly/react-integration@6.0.0-alpha.23
• demo-app-ts@5.1.1-alpha.44
• @patternfly/react-table@6.0.0-alpha.45

Thanks for your contribution! 🎉