feat(Form): add functionality for form group roles #7516

thatblindgeye · 2022-06-06T18:45:47Z

What: Closes #6097, closes #7517

Followup from patternfly/patternfly#4240 and patternfly/patternfly#4424

Form preview build

Added a prop object to the FormGroup component, which when passed in will:

render a span instead of a label element
link the above span to the .pf-c-form__group element via the aria-labelledby attribute
apply either role="group" or role="radiogroup" to the .pf-c-form__group element, depending on whether the multipleInputs.isRadioGroup prop is set to true or not

Also update examples and added a radio form group to more closely resemble the Core example.

Additional issues:

patternfly-build · 2022-06-06T18:55:32Z

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

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

nicolethoen · 2022-06-07T16:49:18Z

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

  fieldId: string;
+  /** Object for identifying whether the form group has multiple inputs for a single label, and
+   * whether the multiple inputs are radio inputs or not. */
+  multipleInputs?: MultipleInputsObject;


I think this is pretty slick - but it does feel very different from anything we've done before with props. I think I like it. I'm curious what others think.

wise-king-sullyman

Overall this looks great! I think the testing is really solid but I did make one suggestion and have one thing I'll need to look into more and get back to you about.

I also noticed one slight typing/demo issue and that's the only real "hard" blocker out of all my comments.

wise-king-sullyman · 2022-06-07T17:21:25Z

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

  fieldId: string;
+  /** Object for identifying whether the form group has multiple inputs for a single label, and
+   * whether the multiple inputs are radio inputs or not. */
+  multipleInputs?: MultipleInputsObject;


In some examples you're passing multipleInputs as a boolean, but it's just typed as an object here. I think that either those examples should be updated to pass an empty object or the type here should be updated to be an object or a boolean.

If you go the type updating route though I feel like this prop name could use a slight edit.

I do also wonder if (as we've talked about a bit) this API would maybe make more sense with two separate props instead of an object prop that wraps another singular prop. I'm not positive how I feel about this idea though and would love input from others 🙂

I definitely understand the concern with the API here. A few ways that we could go about this off the top of my head:

Update the interface so that isRadioGroup is required when passing in multipleInputs, and updating examples so that either multipleInputs={{ isRadioGroup: true }} or multipleInputs={{ isRadioGroup: false }} is passed in (as opposed to just multipleInputs

Renaming the prop to something along the lines of groupOptions or groupType, then adding in another prop to the interface along the lines of inputAmount: 'single' | 'multiple' (more similar to your DataList implementation). This would result in a prop looking like groupOptions={{ inputAmount: 'multiple', isRadioGroup: 'true' }}. Not personally sure if this would make the most sense, however, since the default behavior is already a single input per form group, and this might require passing in props to retain that default behavior.

Just breaking things out to two separate props like you mentioned, maybe hasMultipleInputs and isRadioGroup, and just noting in the isRadioGroup description that it should only be passed in when hasMultipleInputs is also passed in.

Tagging @nicolethoen as well for input on the above proposals

Yeah those are all the direct options I can think of as well, and honestly I don't love any of them. Your current API implementation is probably my favorite of these options.

Is there any way we could automatically determine if a group has multiple inputs rather than requiring the consumer to pass that in as a prop? I feel like that would really be the best approach from the consumer side.

We could possibly check whether the children prop of the Form Group is an array (more than 1 input will return true), but this could possibly get skewed if any non-input content is rendered within the Form Group, such as helper text or validation text. I tinkered around a little and there's some workaround to that, but it may not be 100% accurate all the time due to there being various variables to consider (if it's a native input element, whether it's a React component, whether that React component is an input, etc.)

This is just from some quick tinkering around, though, so I might be missing something more obvious.

Yeah that doesn't seem ideal. I have one vague idea taking shape which would involve in some way adding registration behavior of some sort to our input components. If that would even work though it would probably be out of scope for this issue, so for the moment at least I'm going to join in saying that your implementation is slick and I think I like it.

wise-king-sullyman · 2022-06-07T17:34:31Z

packages/react-core/src/components/Form/__tests__/FormGroup.test.tsx

+
+      expect(labelElement.tagName).toBe('SPAN');
+    });
+  });


One test I would add here: a sanity check test for the expected behavior when multipleInputs is not passed or is otherwise falsy.

wise-king-sullyman · 2022-06-07T17:45:37Z

packages/react-core/src/components/Form/__tests__/FormGroup.test.tsx

+      const labelElement = screen.getByRole('group').querySelector('.pf-c-form__label');
+
+      expect(labelElement.tagName).toBe('SPAN');


This feels very non-RTL to me, but I'll have to think about if there's a way to test this that would be more in line with a real user experience and circle back to you. There very well might not be.

Do you think testing against a snapshot would make more sense here? Once unit tests are rewritten to align better with our current intentions, this would ideally only result in 2 snapshots for the component.

Another possibility might be testing user events. If there's a form group with a single input, clicking the label should place focus on the input; if there's a form group with multiple inputs, however, clicking the span won't place focus anywhere.

Another possibility might be testing user events. If there's a form group with a single input, clicking the label should place focus on the input; if there's a form group with multiple inputs, however, clicking the span won't place focus anywhere.

That seems like it aligns super well with the actual difference in UX and is the way I would go, awesome idea!

wise-king-sullyman

I really like the testing approach here! I just have a couple of small things about the selectors being used.

wise-king-sullyman · 2022-06-08T17:54:06Z

packages/react-core/src/components/Form/__tests__/FormGroup.test.tsx

+    const labelElement = screen.getByTestId('form-group-test-id').querySelector('.pf-c-form__label');
+    const input = screen.getByTestId('form-group-test-id').querySelector('input');


To better replicate the real user experience I would like if we could get away from relying on test ids to select these elements if possible.

I haven't had the time to test this myself yet to confirm, but I think that this should be possible via something like:

Suggested change

const labelElement = screen.getByTestId('form-group-test-id').querySelector('.pf-c-form__label');

const input = screen.getByTestId('form-group-test-id').querySelector('input');

const labelElement = screen.getByText('label');

const input = screen.getByRole('textbox');

Ah good catch! The getByText is more obvious in hindsight, but didn't think of the textbox role 🔥

wise-king-sullyman · 2022-06-08T17:55:24Z

packages/react-core/src/components/Form/__tests__/FormGroup.test.tsx

+      );
+
+      const labelElement = screen.getByRole('group').querySelector('.pf-c-form__label');
+      const inputs = screen.getByRole('group').querySelectorAll('input');


And for selecting multiple inputs you should be able to just use getAllByRole rather than getByRole.

thatblindgeye · 2022-06-09T14:20:27Z

Based on conversation, the latest update renames the new prop to simply "role", and all tests and examples have been updated accordingly.

wise-king-sullyman

I really like this, just one question.

wise-king-sullyman · 2022-06-09T15:43:02Z

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

+  /** Sets the role of the form group if multiple inputs are being passed in. If multiple radio inputs
+   * are passed in use "radiogroup", otherwise use "group" when passing in multiple of any other input.
+   */
+  role?: 'group' | 'radiogroup';


One last thought about this, would it be better to just type this as string and just conditionally check for group or radiogroup explicitly as needed, so that if a consumer wants a different role to apply they can do so?

cc @nicolethoen

I agree - I think it can be string in case someone wants to explicitly set role=none or role=presentation or who knows what else

In terms of the scope of the Form Group fix, if we allow any role to be passed in that might require another update in Core (right now I believe they're only applying either group or radiogroup; EDIT: this may only really be applicable if Core were to show an example of any other role, and even then probably only affects the handlebars file so this could actually be a moot point).

If the intent of a form group is to act more like a fieldset (which by default gets announced as a "group"), only allowing either of those two options seems like the better solution. If we'd want to allow any role to be passed, however, it might also be worth adding a disclaimer in the prop description about using other roles (basically a "be careful" sort of thing).

I think that we should allow any role to be passed through for now at least, since like @nicolethoen mentioned yesterday FormGroup is currently often used in a non-fieldset-like fashion. I also wonder if any consumer might currently be applying a different role to a FormGroup for some reason, in which case limiting it to only group/radiogroup could be breaking.

You bring up good points though, and this seems like a topic that deserves further discussion and maybe a change in the next breaking change release?

That's a fair point. Will update the type to a string, and added this to our breaking change doc just to keep it in mind.

mcarrano

Seems like there is still a problem with the way checkboxes and radio buttons are used in the examples. I should never be able to select multiple radio button options like this:

Can we change this to be consistent with the core demo that looks like this?

wise-king-sullyman

Looks great!

mcarrano

Looks good now. Thanks @thatblindgeye !

mattnolting

LGTM 🎉

srambach

LGTM 👍

tlabaj · 2022-06-14T15:02:16Z

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

+  /** Sets the role of the form group. Pass in "radiogroup" when the form group contains multiple
+   * radio inputs, or pass in "group" when the form group contains multiple of any other input type.
+   */
+  role?: string;


Can this only be radiogroup or group? If so i would suggest defining a union.

It can be more than just radiogroup or group, role is an attribute like aria-label that could be a lot of things. But if it's radio/radiogroup this PR makes it automatically also do some dynamic wiring of a few aria-attributes so that the inputgroups of radio or checkboxes will be labelled correctly.

jessiehuff

LGTM! Tested in VO, JAWS, and NVDA and all announce them as a group as expected. :)

patternfly-build · 2022-06-14T18:55:16Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.61.0
@patternfly/react-catalog-view-extension@4.73.0
@patternfly/react-charts@6.75.0
@patternfly/react-code-editor@4.63.0
@patternfly/react-console@4.73.0
@patternfly/react-core@4.222.0
@patternfly/react-docs@5.83.0
@patternfly/react-icons@4.73.0
@patternfly/react-inline-edit-extension@4.67.0
demo-app-ts@4.182.0
@patternfly/react-integration@4.184.0
@patternfly/react-log-viewer@4.67.0
@patternfly/react-styles@4.72.0
@patternfly/react-table@4.91.0
@patternfly/react-tokens@4.74.0
@patternfly/react-topology@4.69.0
@patternfly/react-virtualized-extension@4.69.0
transformer-cjs-imports@4.60.0

Thanks for your contribution! 🎉

thatblindgeye added ux review A11y labels Jun 6, 2022

thatblindgeye requested review from jessiehuff, mcarrano, mcoker, nicolethoen and wise-king-sullyman June 6, 2022 18:45

thatblindgeye force-pushed the formGroup_roles branch from 965ee6e to 283a8fe Compare June 7, 2022 14:08

nicolethoen mentioned this pull request Jun 7, 2022

chore(Form): convert examples to TypeScript/functional components #7521

Merged

nicolethoen approved these changes Jun 7, 2022

View reviewed changes

wise-king-sullyman requested changes Jun 7, 2022

View reviewed changes

thatblindgeye force-pushed the formGroup_roles branch from 283a8fe to aa7c6b7 Compare June 8, 2022 13:45

thatblindgeye requested a review from wise-king-sullyman June 8, 2022 14:18

wise-king-sullyman reviewed Jun 8, 2022

View reviewed changes

thatblindgeye force-pushed the formGroup_roles branch 2 times, most recently from cf8221a to c5b164c Compare June 9, 2022 14:19

thatblindgeye requested a review from wise-king-sullyman June 9, 2022 14:20

wise-king-sullyman reviewed Jun 9, 2022

View reviewed changes

mcarrano suggested changes Jun 9, 2022

View reviewed changes

Eric Olkowski added 8 commits June 13, 2022 08:22

feat(Form): add functionality for form group roles

bd53a80

Add tests for new multipleInputs prop

514caab

Add group role to form section and field group components

6c1eda8

Update tests for form group component

0b84510

Update tests for checking element of text label

92c40cf

Remove testId in tests

210e553

Refactor implementation of new prop

a34d265

Update examples per request

f5b9ffc

Change prop type to string

7c490d1

thatblindgeye force-pushed the formGroup_roles branch from c5b164c to 7c490d1 Compare June 13, 2022 12:24

thatblindgeye requested review from mcarrano and wise-king-sullyman June 13, 2022 12:25

wise-king-sullyman approved these changes Jun 13, 2022

View reviewed changes

mcarrano approved these changes Jun 13, 2022

View reviewed changes

nicolethoen requested a review from srambach June 13, 2022 15:15

nicolethoen approved these changes Jun 13, 2022

View reviewed changes

mattnolting approved these changes Jun 13, 2022

View reviewed changes

srambach approved these changes Jun 14, 2022

View reviewed changes

tlabaj reviewed Jun 14, 2022

View reviewed changes

jessiehuff approved these changes Jun 14, 2022

View reviewed changes

tlabaj approved these changes Jun 14, 2022

View reviewed changes

tlabaj merged commit 4ff2f54 into patternfly:main Jun 14, 2022

		const labelElement = screen.getByRole('group').querySelector('.pf-c-form__label');

		expect(labelElement.tagName).toBe('SPAN');

		const labelElement = screen.getByTestId('form-group-test-id').querySelector('.pf-c-form__label');
		const input = screen.getByTestId('form-group-test-id').querySelector('input');

feat(Form): add functionality for form group roles #7516

feat(Form): add functionality for form group roles #7516

Uh oh!

Conversation

thatblindgeye commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye commented Jun 9, 2022

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessiehuff left a comment

Choose a reason for hiding this comment

thatblindgeye commented Jun 6, 2022 •

edited

Loading

patternfly-build commented Jun 6, 2022 •

edited

Loading

thatblindgeye Jun 9, 2022 •

edited

Loading