Consume core Penta updates: Form controls, checkbox, radio #10016

adamviktora · 2024-01-17T13:55:05Z

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 · 2024-01-17T14:00:58Z

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

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

thatblindgeye · 2024-02-08T16:36:15Z

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 · 2024-02-20T18:41:09Z

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 · 2024-02-21T13:44:23Z

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 · 2024-03-06T20:48:43Z

packages/react-core/src/components/Form/FormGroupLabelHelp.tsx

+  className,
+  ...props
+}) => (
+  <Button aria-label={ariaLabel} className={className} variant="plain" hasNoPadding {...props}>


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.

@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.

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

tlabaj · 2024-03-11T20:02:27Z

packages/react-core/src/components/Form/FormFieldGroupHeader.tsx

@@ -2,16 +2,13 @@ import * as React from 'react';
 import styles from '@patternfly/react-styles/css/components/Form/form';
 import { css } from '@patternfly/react-styles';

-// typo - rename to FormFieldGroupHeaderTitleTextObject during breaking change release
-export interface FormFiledGroupHeaderTitleTextObject {
+export interface FormFieldGroupHeaderTitleTextObject {


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

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

tlabaj

lgtm

…tion "start" / "end"

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 · 2024-03-26T16:00:40Z

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! 🎉

adamviktora linked an issue Jan 17, 2024 that may be closed by this pull request

Consume core Penta updates: Form controls, checkbox, radio #9931

Closed

adamviktora requested review from a team, mfrances17, kmcfaul, srambach and tlabaj and removed request for a team January 17, 2024 14:33

thatblindgeye force-pushed the form-penta branch from d942c19 to 12d4059 Compare February 8, 2024 16:16

thatblindgeye marked this pull request as ready for review February 8, 2024 16:52

thatblindgeye linked an issue Feb 8, 2024 that may be closed by this pull request

Use plain, no-padding button on form help icon #10055

Closed

2 tasks

adamviktora added the Breaking change 💥 this change requires a major release and has API changes. label Feb 13, 2024

adamviktora mentioned this pull request Feb 13, 2024

FormGroup - Penta updates patternfly/pf-codemods#563

Closed

tlabaj added ux review labels Feb 13, 2024

This was referenced Feb 14, 2024

Checkbox - add examples of "Reversed" and "Label wraps" #10085

Closed

feat(FormGroup): rename labelIcon to labelHelp patternfly/pf-codemods#581

Merged

adamviktora mentioned this pull request Feb 21, 2024

Checkbox / Radio - replace isLabelBeforeButton with labelPosition="start" patternfly/pf-codemods#583

Closed

adamviktora mentioned this pull request Feb 21, 2024

feat(Checkbox/Radio): replace isLabelBeforeButton with labelPosition="start" patternfly/pf-codemods#587

Merged

adamviktora force-pushed the form-penta branch from 7613d4c to 2387f76 Compare February 22, 2024 08:54

wise-king-sullyman self-requested a review March 4, 2024 19:14

wise-king-sullyman approved these changes Mar 4, 2024

View reviewed changes

mcoker approved these changes Mar 6, 2024

View reviewed changes

tlabaj reviewed Mar 11, 2024

View reviewed changes

adamviktora mentioned this pull request Mar 12, 2024

FormFiledGroupHeaderTitleTextObject - typo, rename to FormFieldGroupHeaderTitleTextObject patternfly/pf-codemods#609

Closed

tlabaj approved these changes Mar 12, 2024

View reviewed changes

adamviktora mentioned this pull request Mar 13, 2024

FormGroupLabelHelp: use component="span" and isInline to match core #10158

Closed

adamviktora and others added 8 commits March 26, 2024 09:34

feat(FormGroup): add FormGroupLabelHelp and update examples to use it

e4da81b

docs(FormGroup): recommend using FormGroupLabelHelp

9faa8ea

test(FormGroup): update spaces in snapshots

5a8470b

Updated labelIcon prop name

4c5f0b2

Update prop in example

a417946

feat(Checkbox/Radio): replace isLabelBeforeButton flag with labelPosi…

8963d67

…tion "start" / "end"

test(Radio): update snapshot to v6

41b1c0c

docs(FormFieldGroupHeader): prop description

6225579

adamviktora force-pushed the form-penta branch from 2a83c87 to 6225579 Compare March 26, 2024 08:34

andrew-ronaldson self-requested a review March 26, 2024 15:25

andrew-ronaldson approved these changes Mar 26, 2024

View reviewed changes

wise-king-sullyman merged commit 59d2df2 into patternfly:v6 Mar 26, 2024
13 checks passed

This was referenced Mar 26, 2024

Consume core Penta updates: Form controls, checkbox, radio #9931

Closed

Use plain, no-padding button on form help icon #10055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume core Penta updates: Form controls, checkbox, radio #10016

Consume core Penta updates: Form controls, checkbox, radio #10016

adamviktora commented Jan 17, 2024 •

edited

Loading

patternfly-build commented Jan 17, 2024 •

edited

Loading

thatblindgeye commented Feb 8, 2024 •

edited

Loading

kmcfaul commented Feb 20, 2024

adamviktora commented Feb 21, 2024

mcoker left a comment

mcoker Mar 6, 2024

adamviktora Mar 7, 2024 •

edited

Loading

mcoker Mar 7, 2024

tlabaj Mar 11, 2024

adamviktora Mar 12, 2024

tlabaj left a comment

andrew-ronaldson left a comment

patternfly-build commented Mar 26, 2024

Consume core Penta updates: Form controls, checkbox, radio #10016

Consume core Penta updates: Form controls, checkbox, radio #10016

Conversation

adamviktora commented Jan 17, 2024 • edited Loading

patternfly-build commented Jan 17, 2024 • edited Loading

thatblindgeye commented Feb 8, 2024 • edited Loading

kmcfaul commented Feb 20, 2024

adamviktora commented Feb 21, 2024

mcoker left a comment

Choose a reason for hiding this comment

mcoker Mar 6, 2024

Choose a reason for hiding this comment

adamviktora Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

mcoker Mar 7, 2024

Choose a reason for hiding this comment

tlabaj Mar 11, 2024

Choose a reason for hiding this comment

adamviktora Mar 12, 2024

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

andrew-ronaldson left a comment

Choose a reason for hiding this comment

patternfly-build commented Mar 26, 2024

adamviktora commented Jan 17, 2024 •

edited

Loading

patternfly-build commented Jan 17, 2024 •

edited

Loading

thatblindgeye commented Feb 8, 2024 •

edited

Loading

adamviktora Mar 7, 2024 •

edited

Loading