chore(Form): convert examples to TypeScript/functional components by andyyvo · Pull Request #7521 · patternfly/patternfly-react

andyyvo · 2022-06-07T01:53:16Z

What: Closes #7466 . Closes #5767 . Converted all Form component React demo examples into TypeScript. Two things to watch: FormInvalid.tsx and FormInvalidWithFormAlert.tsx swapped noval to default; FormValidated.tsx originally had a small bug where the setTimeout callback would stack based on the number of key inputs, but was fixed using useEffect.

Additional issues: N/A

patternfly-build · 2022-06-07T02:05:28Z

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

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

nicolethoen · 2022-06-07T13:48:31Z

packages/react-core/src/components/Form/examples/FormBasic.tsx

+import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
+
+export const SimpleForm: React.FunctionComponent = () => {
+  const [value1, setValue1] = React.useState(''); // name


Might it be easier to name the state variables as name or even nameValue instead of value1?

across all examples

+1 @nicolethoen

nicolethoen · 2022-06-07T13:50:31Z

packages/react-core/src/components/Form/examples/FormInvalidWithFormAlert.tsx

+      setValidated('success');
+    } else {
+      setValidated('error');
+    } // changed noval to default


these comments are not necessary, even though they are helpful when reviewing. Future consumers who will be looking at this example code as a guide when implementing forms and won't have the context to know what you're referring to.

Could you clean up this comment?

nicolethoen · 2022-06-07T14:28:59Z

packages/react-core/src/components/Form/examples/FormHorizontalHelper.tsx

+        helperText={helperText}
+        isHelperTextBeforeField
+        hasNoPaddingTop
+        fieldId="options"


The fieldId prop should map to the id of the form element to which a label is bound. In cases like this one there is no corresponding input or element with the specified id.

I think ideally, in these cases we'd be using a fieldset element and it's associated legend rather than a div.pf-c-form-group and (unofficially) associated label. Here is w3c article for reference. I have opened an issue in core to consider the markup updates.

For now, we may as well remove the fieldId from the places where it is trying to label multiple controls at once.

I think that eric is actually resolving this exact issue in this PR: #7516 so disregard this :)

nicolethoen · 2022-06-07T14:31:48Z

packages/react-core/src/components/Form/examples/Form.md

-    );
-  }
-}
+```ts file="./FormGrid.tsx"


I think you meant to link to the ./FormSections.tsx file

jenny-s51

This is looking great @andyyvo! Nice update to the Validated example with useEffect 👍

I agree with @nicolethoen and was just about to comment the same - for best practice + readability, it would be good to update the hook variable names and value change methods with names that correspond to the fields directly!

Nice work on this!

jenny-s51 · 2022-06-07T14:22:01Z

packages/react-core/src/components/Form/examples/FormGroupLabelInfo.tsx

+import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
+
+export const FormGroupLabelInfo: React.FunctionComponent = () => {
+  const [value1, setValue1] = React.useState(''); // name


Looks like you only have one field here, so you could probably just go with value or name!

Suggested change

const [value1, setValue1] = React.useState(''); // name

const [name, setName] = React.useState('');

jenny-s51 · 2022-06-07T14:25:23Z

packages/react-core/src/components/Form/examples/FormLimitWidth.tsx

+import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
+
+export const SimpleForm: React.FunctionComponent = () => {
+  const [value1, setValue1] = React.useState(''); // name


Suggested change

const [value1, setValue1] = React.useState(''); // name

const [name, setName] = React.useState('');

jenny-s51 · 2022-06-07T14:41:23Z

packages/react-core/src/components/Form/examples/FormBasic.tsx

+  const [value2, setValue2] = React.useState(''); // email
+  const [value3, setValue3] = React.useState(''); // phone
+
+  const handleValue1Change = value1 => {


Suggested change

const handleValue1Change = value1 => {

const handleNameChange = name => {

andyyvo · 2022-06-07T15:10:24Z

Sounds great, thanks @nicolethoen and @jenny-s51 !! I have a few questions that I'd love to learn more about for future reference.

When it comes to naming conventions, should the state variable (and corresponding handler function) names be written based on their respective input? For example, the Full Name input uses const [name, setName] and Email uses const [email, setEmail]. I'd love to know how to best approach components with one state variable vs. a couple state variables vs. many state variables (Field groups).
When making changes to an existing PR, should I go back into my code editor, make the changes there, and then push a new PR? I hope I don't sound like too much of a novice but while I've worked with Git for awhile, class projects usually don't use PRs often.

Thank you so much!!

jenny-s51 · 2022-06-07T15:36:41Z

Great questions @andyyvo !!

Generally we want to keep things as clear and readable as possible, since as Nicole said, consumers use our demo code as a guide for their implementations of our components! It looks like Nicole recently made some updates to Modal with Form using the same kind of naming conventions we suggested here.

Regarding the Field Groups demo, you raise an interesting question since there are 22 inputs. Some of those are nested into other groups, so it seems to make more sense to leave the variable names as-is (e.g. value1, value2, etc.). Otherwise we'd have some pretty verbose variable names e.g. FieldGroup1Label1, NestedFieldGroup1Label1, NestedFieldGroup1Label2, NestedFieldGroup1Label1, NestedFieldGroup2Label1, etc. which seems like it'd be more difficult for the consumer to interpret. WDYT @nicolethoen ?
In order to update an existing PR with your latest changes, you can go back to your code editor, make the corresponding updates, then:
git add [files you changed] -> git commit -m '[what you changed]' -> git push origin iss7466!

That should just push right to this PR without you needing to open a new one 🙂

andyyvo · 2022-06-08T14:57:13Z

Just added all the cleaned up files. FormFieldGroups.tsx is still something that needs work. I'm running into an issue with object destructuring. It appears that handleChange is running into an issue on line 65 where it can't read name. I think the problem is that in the HTML attributes where I'm referencing handleChange, the name attribute for TextInput has hyphens in the string.

andyyvo · 2022-06-08T17:45:35Z

Just wrapped up FormFieldGroups.tsx. Thank you so much, @nicolethoen !!!

nicolethoen

Looks great! one nit

nicolethoen · 2022-06-08T18:27:06Z

packages/react-core/src/components/Form/examples/FormInvalidWithFormAlert.tsx

+      setValidated('success');
+    } else {
+      setValidated('error');
+    } // changed noval to default


Could you clean up this comment?

tlabaj · 2022-06-13T18:51:30Z

@andyyvo can you verify that this issue is fixed with this PR please. If not, can you take a look at it sine you are in the code?
#5767

andyyvo · 2022-06-13T19:30:36Z

@nicolethoen is #5767 the issue that you, me, and Eric were discussing about fieldID or fieldGroups?

nicolethoen · 2022-06-14T12:34:42Z

@andyyvo it is not - I was talking about this core issue

jenny-s51

This is looking awesome! Everything seems to work as expected. Left a comment about the initialValues variable names - I think this demo would make a lot more sense to consumers if we cleaned those up a bit 🙂 Should be an easy update! WDYT @andyyvo @nicolethoen ?

jenny-s51 · 2022-06-15T20:12:28Z

packages/react-core/src/components/Form/examples/FormFieldGroups.tsx

+    'form-expandable-field-groups-field-group2-label2': '',
+    'form-expandable-field-groups-field-group3-label1': '',
+    'form-expandable-field-groups-field-group3-label2': '',
+    'form-expandable-field-groupsform-expandable-field-groups-field-group1-label1': '',


I think form-expandable-field-groupsform-expandable-field-groups-field-group1-label1 is referring to one of the nested fields?

We could call it field-group1-nested-group1-label1 - would make it more clear 🙂

jenny-s51 · 2022-06-15T20:26:03Z

packages/react-core/src/components/Form/examples/FormFieldGroups.tsx

+    'form-expandable-field-group2-label2': '',
+    'form-expandable-field-group3-label1': '',
+    'form-expandable-field-group3-label2': '',
+    'form-expandable-field-groups-field-group7-label1': '',


Same idea for these... it looks like in the demo there's no "Field group 7", "Field Group 8", or "Field group 10" so it's tricky to understand which inputs these are referring to.

We could have something like field-group3-nested-field-group1-label1 here - WDYT?

jenny-s51 · 2022-06-15T20:27:31Z

packages/react-core/src/components/Form/examples/FormFieldGroups.tsx

+
+export const FieldGroups: React.FunctionComponent = () => {
+  const initialValues = {
+    'form-expandable-field-groups-label1': '',


Is there any way we could clean these names up? Would be a lot more DRY and readable if we removed the form-expandable-field-groups- text from all of these!

Suggested change

'form-expandable-field-groups-label1': '',

'label1': '',

Looks good and I totally agree @jenny-s51 ! The only issue I ran into with this was that the names of the labels in initialValues had to match the id and other props in the components used in order for the state variables to update the input. If I'm able to, I can change all of the id props to match it to something simpler?

That sounds like a good plan @andyyvo !

nicolethoen · 2022-06-16T19:41:20Z

packages/react-core/src/components/Form/examples/FormBasic.tsx

+          onChange={handlePhoneChange}
+        />
+      </FormGroup>
+      <FormGroup role="group" isInline fieldId="basic-form-checkbox-group" label="How can we contact you?">


I don't think these fieldId props are service a purpose when you set a role

I believe that's what the other issue is referencing. If you clean these up you can mark the other issue addressed.

Should I remove all fieldId where there is a role then? The fieldId prop is currently required in FormGroup components

@nicolethoen can we imagine a case where a consumer would need/want to pass in their own ID for the form group label? If not, an alternative to having this fieldId prop setting two different things is to just have a random ID created internally to set the aria-labelledby attribute whenever role="group" or role="radiogroup" is passed in.

jenny-s51

This is looking awesome @andyyvo !! Recent updates are excellent 👌

Left a couple comments about some small naming-related things - last few updates I think! Once those are addressed this PR should be good to 🚀!!

jenny-s51 · 2022-06-17T15:10:09Z

packages/react-core/src/components/Form/examples/FormFieldGroups.tsx

+    'group3-label1': '',
+    'group3-label2': '',
+    'exapnded-group1-label1': '',
+    'exapnded-group1-label2': '',


Looks like we just need to make this is fixed throughout FormFieldGroups.tsx! I'm seeing this in a few places

Suggested change

'exapnded-group1-label2': '',

'expanded-group1-label2': '',

jenny-s51 · 2022-06-17T15:17:05Z

packages/react-core/src/components/Form/examples/FormFieldGroups.tsx

+            isRequired
+            id="exapnded-group2-label1"
+            name="exapnded-group2-label1"
+            value={inputValues['exapnded-group2-label1']}


We should make sure the non-expandable example doesn't have a field with expanded in the name

sounds good! I'll get right on that :D

jenny-s51

Thanks @andyyvo !! Great job on this - LGTM 🤩

nicolethoen

Just need to resolve merge conflicts 👍

nicolethoen

Looks like the a11y tests failed because of this:

@patternfly/react-docs: 240/873   /components/form/react/basic
@patternfly/react-docs: violations: duplicate-id-aria
@patternfly/react-docs: 241/873   /components/form/react/horizontal
@patternfly/react-docs: violations: duplicate-id-aria
@patternfly/react-docs: 242/873   /components/form/react/limit-width
@patternfly/react-docs: violations: duplicate-id-aria

More information here: https://patternfly-react-pr-7521-a11y.surge.sh/

andyyvo · 2022-06-18T00:30:18Z

It appears that the accessibility issue derives from role and fieldId. More specifically, because role prop essentially removes the purpose of fieldId, we no longer define a fieldId. However, FormGroup.tsx uses the fieldId prop to create aria-labelledby and id (as seen on line 126 and 136 respectively of FormGroup.tsx of my most recent commit). Since there's no fieldId, it is undefined. This is resulting in a duplicate of aria-labelledby and id for the two instances of checkboxes/radio buttons in FormBasic.tsx, FormHorizontal.tsx, and FormLimitWidth.tsx. One possible solution I have in mind is to get rid of one example in each. However, that would still conflict for the consumer if they do try to implement both. A solution I just discovered is to define a fieldId when the role prop is used because fieldId is still processed for props mentioned before.

…edby

thatblindgeye

This is looking good! Had a couple of comments below which are dependent on responses from those I tagged.

One thing that can be updated is the name of the export in each example file. Those should match the file name, so for FormBasic.tsx, the export should be export const FormBasic: React.FunctionComponent... and so on.

thatblindgeye · 2022-06-21T13:04:18Z

packages/react-core/src/components/Form/examples/FormInvalid.tsx

+import { Form, FormGroup, TextInput, FormHelperText } from '@patternfly/react-core';
+import ExclamationCircleIcon from '@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon';
+
+export const InvalidForm: React.FunctionComponent = () => {


@mcarrano based on your comment for a similar case at #7489 (comment), should the Invalid example for the Form component also be removed since we have the Validated example already?

thatblindgeye · 2022-06-21T13:28:13Z

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

  /** ID of the included field. It has to be the same for proper working. */
-  fieldId: string;
+  fieldId?: string;


If we decide to have this prop continue working as-is, i.e. setting that aria-labelledby on FormGroup, then this description could use updating. Something along the lines of below, but I'd hold off until Nicole responds to my other comment regarding this prop.

"ID of an individual field or a group of multiple fields. Required when a role of "group" or "radiogroup" is passed in. If only one field is included, its ID attribute and this prop must be the same."

thatblindgeye · 2022-06-21T13:38:00Z

packages/react-core/src/components/Form/examples/FormBasic.tsx

+          onChange={handlePhoneChange}
+        />
+      </FormGroup>
+      <FormGroup role="group" isInline fieldId="basic-form-checkbox-group" label="How can we contact you?">


@nicolethoen can we imagine a case where a consumer would need/want to pass in their own ID for the form group label? If not, an alternative to having this fieldId prop setting two different things is to just have a random ID created internally to set the aria-labelledby attribute whenever role="group" or role="radiogroup" is passed in.

…generate id for forms

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

packages/react-core/src/components/Form/examples/FormGroupLabelInfo.tsx

…example

thatblindgeye

🎉 Looks good!

…tternfly#7521) * chore(Form): completed conversion of form examples to ts * chore(Form): added fieldId back to roles to define id and aria-labelledby * chore(Form): renamed form functions, updated fieldId desc, and added generate id for forms * chore(Form): cleaned FormGroup logic and added fieldId to label info example

nicolethoen requested changes Jun 7, 2022

View reviewed changes

jenny-s51 suggested changes Jun 7, 2022

View reviewed changes

andyyvo changed the title ~~completed conversion of form examples to ts~~ chore(Form): convert examples to TypeScript/functional components Jun 7, 2022

nicolethoen reviewed Jun 8, 2022

View reviewed changes

nicolethoen approved these changes Jun 10, 2022

View reviewed changes

jenny-s51 suggested changes Jun 15, 2022

View reviewed changes

nicolethoen reviewed Jun 16, 2022

View reviewed changes

jenny-s51 suggested changes Jun 17, 2022

View reviewed changes

jenny-s51 approved these changes Jun 17, 2022

View reviewed changes

nicolethoen requested changes Jun 17, 2022

View reviewed changes

chore(Form): completed conversion of form examples to ts

ea61a19

andyyvo force-pushed the iss7466 branch from 35e9d75 to ea61a19 Compare June 17, 2022 18:26

nicolethoen approved these changes Jun 17, 2022

View reviewed changes

nicolethoen requested changes Jun 17, 2022

View reviewed changes

chore(Form): added fieldId back to roles to define id and aria-labell…

a89f083

…edby

thatblindgeye requested changes Jun 21, 2022

View reviewed changes

chore(Form): renamed form functions, updated fieldId desc, and added …

709aa34

…generate id for forms

thatblindgeye requested changes Jun 23, 2022

View reviewed changes

packages/react-core/src/components/Form/FormGroup.tsx Outdated Show resolved Hide resolved

packages/react-core/src/components/Form/FormGroup.tsx Outdated Show resolved Hide resolved

packages/react-core/src/components/Form/examples/FormGroupLabelInfo.tsx Show resolved Hide resolved

chore(Form): cleaned FormGroup logic and added fieldId to label info …

ff5558a

…example

thatblindgeye approved these changes Jun 23, 2022

View reviewed changes

nicolethoen approved these changes Jun 23, 2022

View reviewed changes

nicolethoen merged commit 19f4672 into patternfly:main Jun 23, 2022

	const [value1, setValue1] = React.useState(''); // name
	const [name, setName] = React.useState('');

	const handleValue1Change = value1 => {
	const handleNameChange = name => {

Conversation

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

Uh oh!

Uh oh!

patternfly-build commented Jun 7, 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!

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!

jenny-s51 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

andyyvo commented Jun 7, 2022

Uh oh!

jenny-s51 commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyyvo commented Jun 8, 2022

Uh oh!

andyyvo commented Jun 8, 2022

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Jun 13, 2022

Uh oh!

andyyvo commented Jun 13, 2022

Uh oh!

nicolethoen commented Jun 14, 2022

Uh oh!

jenny-s51 left a comment

Choose a reason for hiding this comment

Uh oh!

jenny-s51 Jun 15, 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!

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!

andyyvo Jun 16, 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!

jenny-s51 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

andyyvo commented Jun 7, 2022 •

edited

Loading

patternfly-build commented Jun 7, 2022 •

edited

Loading

jenny-s51 left a comment •

edited

Loading

jenny-s51 commented Jun 7, 2022 •

edited

Loading

jenny-s51 Jun 15, 2022 •

edited

Loading

andyyvo Jun 16, 2022 •

edited

Loading

jenny-s51 left a comment •

edited

Loading

jenny-s51 Jun 17, 2022 •

edited

Loading

jenny-s51 left a comment •

edited

Loading