Ux-157 FormElement api simplification #663

tristanjasper · 2020-08-21T20:12:08Z

Purpose 🚀

Proposal for FormElement api simplification so accessibility is handled by the component itself rather than the consumer.

Previously a render function was required as a child of FormElement.Content in order to expose accessibility tags idForLabel, ariaDescribedBy and refLabel which were not obviously available to the developer or how to use them, without looking into the details and storybook examples as each paprika component uses those tags differently.

This PR now takes care this by applying the accessibility tags automatically no matter what component you pass it as a child. This means it is less developer effort to work with.

It is still expected that any other aria-labels such as aria-required should still be supplied by the consumer.

When passing more than one Input or input element label focus

It is expected that in a follow up PR a follow up Api change to allow overriding the behaviour of the default formElement labelId being applied to the first Input.

Additional changes:
Added support for TimePicker with formElement

Notes ✏️

details of code change / secondary purposes of this PR

Updates 📦

MAJOR (breaking) change to formElement
MINOR (backward compatible) change to these packages
PATCH (bug fix) change to TimePicker, Checkbox, DatePicker

Storybook 📕

http://storybooks.highbond-s3.com/paprika/ux-157-form-element-simplification

Screenshots 📸

optional but highly recommended

References 🔗

https://aclgrc.atlassian.net/browse/UXD-157

tristanjasper · 2020-08-21T20:21:36Z

packages/TimePicker/src/TimePicker.js

@@ -178,7 +178,6 @@ function TimePicker(props) {
            mm={time.mm}
            onClick={handleClick}
            period={time.period}
-            {...moreProps}


moreProps should be getting spread on the root TimePicker component rather than on the trigger input. I wonder why isn't the TimePicker component using a similar api to the DatePicker in that the Trigger Input is passed as a child? We would then be able to set id and aria-describedBy with the same pattern as DatePicker. The alternative would be to add an additional prop of triggerInputProps to the TimePicker component, however I'm not sure that this would be preferable

you can add a new subcomponent <TimePicker.Trigger /> to override the props of the component I guess when was built there was not a need for it. But I don't thinks the point for this PR.

nahumzs · 2020-08-21T22:03:20Z

Did you explore to have a wrapper for transform something like this:

      <FormElement label="Form Label">
        <FormElement.Instructions>
          <span>
            Example text for extra panel for questionnaires. Example text for extra panel for questionnaires Example
            text for extra panel for questionnaires. Example text for extra panel for questionnaires
          </span>
        </FormElement.Instructions>
        <FormElement.Content>
          <Input
            onChange={handleChange}
            value={value}
            placeholder="Form placeholder"
            hasError={Boolean(errorText.length)}
          />
        </FormElement.Content>
        <FormElement.Description>
          <span>This is description text</span>
        </FormElement.Description>
        <FormElement.Error>{errorText}</FormElement.Error>
        <FormElement.Help>
          Give me some help. <a href="wegalvanize.com">Learn more</a>.
        </FormElement.Help>
      </FormElement>

into:

<FormElementShort 
  label="Form Label"
  instructions="..."
  description="..."
  error="..."
  help="..."
>
  <Input
     onChange={handleChange}
     value={value}
     placeholder="Form placeholder"
     hasError={Boolean(errorText.length)}
  />
</FormElementShort>

^ the previous wrapper could be an addition to our current API, is short and in the background will be rendering our current FormElement API

nahumzs · 2020-08-21T22:12:34Z

how are we managing currently if we have two inputs in the same <FormElement /> like:

[name] [lastname]

instead of

[name]
[lastName]

Unsure what you mean? They can be styled to be displayed inline if necessary. Or I'm not understanding your question.

tristanjasper · 2020-08-24T16:01:56Z

Did you explore to have a wrapper for transform something like this:

      <FormElement label="Form Label">
        <FormElement.Instructions>
          <span>
            Example text for extra panel for questionnaires. Example text for extra panel for questionnaires Example
            text for extra panel for questionnaires. Example text for extra panel for questionnaires
          </span>
        </FormElement.Instructions>
        <FormElement.Content>
          <Input
            onChange={handleChange}
            value={value}
            placeholder="Form placeholder"
            hasError={Boolean(errorText.length)}
          />
        </FormElement.Content>
        <FormElement.Description>
          <span>This is description text</span>
        </FormElement.Description>
        <FormElement.Error>{errorText}</FormElement.Error>
        <FormElement.Help>
          Give me some help. <a href="wegalvanize.com">Learn more</a>.
        </FormElement.Help>
      </FormElement>

into:

<FormElementShort 
  label="Form Label"
  instructions="..."
  description="..."
  error="..."
  help="..."
>
  <Input
     onChange={handleChange}
     value={value}
     placeholder="Form placeholder"
     hasError={Boolean(errorText.length)}
  />
</FormElementShort>

^ the previous wrapper could be an addition to our current API, is short and in the background will be rendering our current FormElement API

No I didn't look into that, This approach is new to me, I've never heard of adding additional shortcut components in any discussions. Perhaps elaborate on the reasons for it so we can have an open discussion? If we like it perhaps there are other components that could benefit from a similar shortcut wrapper approach? For me it appears to be adding non essential components though.

nahumzs · 2020-08-24T21:27:01Z

packages/FormElement/src/components/Content/Content.js


  if (!children) {
    return null;
  }

  return (
    <sc.ContentContainer data-pka-anchor="form-element.content" {...moreProps}>
-      {typeof children === "function" ? children({ idForLabel, refLabel, ariaDescribedBy }) : children}
+      {renderExtractedChildren()}


add a console.error or throw new Error if typeof children === "function"

tristanjasper · 2020-08-26T18:28:46Z

packages/FormElement/src/components/Content/renderFunctions.js

+export default {
+  Input: ({ extractedElements, idForLabel, ariaDescribedBy }) =>
+    getAccessibleElement(extractedElements, (element, index = 0) =>
+      cloneWithAriaAttributes({ element, idForLabel: index === 0 ? idForLabel : null, ariaDescribedBy })


This behaviour of the first Input being provided the id (that is associated with the formElement label) will be able to be overridden in a follow up pr

Also on line https://github.com/acl-services/paprika/pull/663/files#diff-1a53d8778ef9b8807972d1aa4463c8c9R42

nahumzs · 2020-08-26T18:32:12Z

packages/FormElement/CHANGELOG.md

@@ -49,3 +49,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 -- Changed the spacing between label, content and instructions

 [@tristanjasper](https://github.com/tristanjasper).
+
+## [0.3.29] - 2020-08-20


this you should follow the new format that @allison-c introduced, write this on the [unreleased] section read in our slack channel in paprika the announcement about it 🙏 :)

nahumzs · 2020-08-26T18:32:50Z

packages/FormElement/README.md

-        size={size}
-      />
-    )}
+    <Input


is this a breaking change or our FormElements still support the renderFunction?

This will be a breaking change. I don't think we should encourage using the renderFunction as an acceptable api moving forward

nahumzs · 2020-08-26T18:35:33Z

packages/FormElement/src/components/Content/Content.js

 };

 const defaultProps = {
  idForLabel: null,
  ariaDescribedBy: null,
  refLabel: null,
+  wrapperAriaDescribedBy: null,


why do we have ariaDescribedBy and wrapperAriaDescribedBy, shouldn't this be part of a more universal prop named a11yText (this is what we use across our library)?

Do you have an example where we are using wrapperAriaDescribedBy?

You can see how we use it in this storybook example - https://github.com/acl-services/paprika/pull/663/files#diff-1a53d8778ef9b8807972d1aa4463c8c9R54

In the case of wrapping FormElements with other FormElements we need to make sure that the aria-describedBy strings coming from the wrapping FormElement are being passed down to the children

Perhaps a better name for these props could be a11yDescribedByIds so I'll switch to that

alexzherdev · 2020-08-26T20:59:20Z

Critiquing without offering an alternative is unfair but I'll try to offer at least something at the end 😄 I want to throw in a general idea: I think it's commendable to try to ease the burden for consumers, but this should always be weighed against complexity or potential for issues within the library. If the library's code becomes too convoluted or otherwise hard to maintain, we're ultimately doing a disservice to the consumers.

Particularly in this case, one thing I'm seeing is we're essentially making Content depend (in the npm/package sense) on all the different form components (input, checkbox, etc.). It's subtle, but the code that knows how to properly clone all the different kinds of components assumes a certain API (especially children) of those components. This is most obvious with DatePicker: the cloning code used by Content knows that DatePicker has a single child which is the DatePicker's "input". If we change that API in a major bump for DatePicker, Content will break when used in conjunction with that new version.

This isn't a normal dependency though since FormElement doesn't import or compose the form components. It would be a peer dependency, because it's really saying hey, this version of me works in the presence of these versions of all those other components. It would actually be an optional peer dependency since using FormElement doesn't require each of the form components to be present. But honestly having any such sort of dependency seems weird.

I don't see a perfect way out, I feel like we may have painted ourselves into a corner introducing this generic FormElement component. The best thing I can see would be for us to rename the properties passed into the render prop so that they can be spread onto the child component without destructuring like so:

<FormElement.Content>
  {a11yProps => (
    <Input
      {...a11yProps}
      {...otherProps}
    />
  )}
</FormElement.Content>

where a11yProps is { id: "someId", "aria-describedby": "something else" }. Just makes it a bit less tedious.

tristanjasper · 2020-08-26T21:10:05Z

Critiquing without offering an alternative is unfair but I'll try to offer at least something at the end 😄 I want to throw in a general idea: I think it's commendable to try to ease the burden for consumers, but this should always be weighed against complexity or potential for issues within the library. If the library's code becomes too convoluted or otherwise hard to maintain, we're ultimately doing a disservice to the consumers.

Particularly in this case, one thing I'm seeing is we're essentially making Content depend (in the npm/package sense) on all the different form components (input, checkbox, etc.). It's subtle, but the code that knows how to properly clone all the different kinds of components assumes a certain API (especially children) of those components. This is most obvious with DatePicker: the cloning code used by Content knows that DatePicker has a single child which is the DatePicker's "input". If we change that API in a major bump for DatePicker, Content will break when used in conjunction with that new version.

This isn't a normal dependency though since FormElement doesn't import or compose the form components. It would be a peer dependency, because it's really saying hey, this version of me works in the presence of these versions of all those other components. It would actually be an optional peer dependency since using FormElement doesn't require each of the form components to be present. But honestly having any such sort of dependency seems weird.

I don't see a perfect way out, I feel like we may have painted ourselves into a corner introducing this generic FormElement component. The best thing I can see would be for us to rename the properties passed into the render prop so that they can be spread onto the child component without destructuring like so:
<FormElement.Content>
  {a11yProps => (
    <Input
      {...a11yProps}
      {...otherProps}
    />
  )}
</FormElement.Content>
where a11yProps is { id: "someId", "aria-describedby": "something else" }. Just makes it a bit less tedious.

Yes, the reason the api is the way it is currently is exactly for that reason (we didn't want to tie the accessibility implementation too tightly to the components using it ) I see positives and negatives from both approaches.

Potentially that solution could be a good option as well as the proposal I have in this pr @alexzherdev. I think that could keep the separation of components, and make it a little bit easier for consumers to implement.

There are some issues there though, in that not in all cases do be want to spread all of the a11yProps onto all children, but we could use that as a generic usage approach I think...

I'll consider some alternatives to the api as well if we were to retain the render function approach

tristanjasper · 2020-08-27T23:04:09Z

Closing this pr as want to reconsider the approach for api simplification.

tristanjasper · 2020-09-01T22:22:58Z

packages/FormElement/src/components/Content/Content.js

+  };
+
+  if (!extractedChildren?.children.length) {
+    delete extractedChildren.children;


extractedChildrenHelper generates an empty children array when not needed, perhaps something we should prevent from happening

tristanjasper · 2020-09-01T22:36:25Z

@alexzherdev Following on from your concerns in comment - #663 (comment)

I've modified certain paprika components (DatePicker, TimePicker) to expect a consistent application of a11yDescribedByIds, therefore if we do make changes to those components in future this shouldn't be too much of an issue of it being too tightly coupled to the formElements approach.

tristanjasper · 2020-09-01T22:43:53Z

packages/TimePicker/src/TimePicker.js

@@ -149,20 +166,21 @@ function TimePicker(props) {

  const { t } = useI18n();
  return (
-    <sc.TimePicker onFocus={handleFocus} onBlur={handleBlur}>
+    <sc.TimePicker onFocus={handleFocus} onBlur={handleBlur} {...moreProps}>


spreading moreProps here was the original intention so updated this (confirmed with @KaanDarcey)

tristanjasper · 2020-09-01T22:46:01Z

packages/FormElement/stories/examples/AlternateLayouts.js

+            isDisabled={isDisabled}
+            isReadOnly={isReadOnly}
+            size={size}
+            overridelabelassociation="true" // Add to override default label association


this is a proposal for how we can override which element is associated with the label, perhaps wrapping with a FormElement.AssociateLabel component could be another approach, or any better proposals?

nahumzs · 2020-09-02T16:54:59Z

I think we have hit a point with this Component where is starting to be clear that our current approach doesn't escalate correctly neither play well for our consumer as being easier to use or an easy good pattern to compose our Form requirements. Currently is verbose and keep going in this direction will add unnecessary complexity to the FormElement.

In the previous attempt we discussed about this propose approach and agree wiring our custom inputs component to this FormElement will make it easy to break it and hard to maintain.
And for the previous reason the render function was introduce, but that created a more "verbose" use of the component where you have to write multiple lines just to be able to use a simple input

Since the idea is to simplify this component, maybe would worth to take a step back approach and make a table of the requirements we need to support with this component and rethink what would be the best solutions for this.

I think pushing this component to try to do what we want in their current form might not the best decision.

Maybe is time to check https://github.com/adobe/react-spectrum/tree/main/packages/%40react-spectrum and see if we can adopt it or get inspiration how they solve this issue.

Another possible solution would be totally detach every piece of this component and provide helpers that generates ids to make easier to compose these elements.

like:

import ListBox from "@paprika/listbox"
import Input from "@paprika/listbox"
import { Label, Description, Help, Error, etc, Elements, useFormElement } from "@paprika/formElement"

const { generateA11yProps, isVisuallyHiden } = useFormElement();

const inputA11yProps = generateA11yProps();
const listBoxA11yProps = generateA11yProps();

<Element.Group kind={Element.types.kind.horizontal}>
   <Element>
     <Label {...inputA11yProps.label}>Name</Label>
     <Input {...inputA11yProps.input} />
   </Element>
   <Element>
     <Label {...listBoxA11yProps} isVisuallyHidden>preference</Label>
     <ListBox {...listBoxA11yProps} {...stuff} />
     <Description>Some description</Description>
    <Element />
<Element.Group>

NOTE The previous might required to adjust all of our custom inputs to recieved all a11yProps at certain level and in a correct form. but we can improve that.

alexzherdev · 2020-09-02T17:08:03Z

I generally agree with the above. What we're trying to do is the opposite of what's recommended here https://www.youtube.com/watch?v=AiJ8tRRH0f8 (which I agree with)

tristanjasper · 2020-09-02T17:55:35Z

Ok I'll look into the above route

tristanjasper added 5 commits August 20, 2020 21:46

initial simplification

1d5a359

cleanup

8539d9a

added support for timepicker

e05071f

Merge branch 'master' into ux-157-form-element-simplification

de5ee5c

prop rename

a28ab6f

tristanjasper commented Aug 21, 2020

View reviewed changes

tristanjasper changed the title ~~Ux 157 form element api simplification~~ Ux 157 FormElement api simplification Aug 21, 2020

tristanjasper changed the title ~~Ux 157 FormElement api simplification~~ Ux-157 FormElement api simplification Aug 21, 2020

rename for boolean variable

659f47e

more performant rendering

0e1933b

nahumzs reviewed Aug 24, 2020

View reviewed changes

tristanjasper added 3 commits August 26, 2020 10:26

move component render calls to external file

9a33dfa

Merge branch 'master' into ux-157-form-element-simplification

3bfbab4

change default props

79569f4

tristanjasper commented Aug 26, 2020

View reviewed changes

tristanjasper requested a review from nahumzs August 26, 2020 18:29

nahumzs reviewed Aug 26, 2020

View reviewed changes

tristanjasper added 2 commits August 26, 2020 13:18

ensure ariaDescribedby as prop on checkbox and rad

22ba49b

update changelog

c4b52d9

tristanjasper closed this Aug 27, 2020

tristanjasper added 2 commits August 28, 2020 09:02

Merge branch 'master' into ux-157-form-element-simplification

6de6997

first pass simplify a11y props

934aa9e

tristanjasper added 4 commits September 1, 2020 11:30

change to support simpler elements

d3562fe

cleanup

1451e3f

add console warnings for incorrect api

3ac7b93

Merge branch 'master' into ux-157-form-element-simplification

350731c

tristanjasper reopened this Sep 1, 2020

tristanjasper commented Sep 1, 2020

View reviewed changes

tristanjasper requested review from nahumzs and alexzherdev September 1, 2020 23:30

Merge branch "master" into ux-157-form-element-simplification

7a5c581

tristanjasper closed this Sep 2, 2020

tristanjasper mentioned this pull request Oct 29, 2020

UXD-157 formElement component api refactor #774

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ux-157 FormElement api simplification #663

Ux-157 FormElement api simplification #663

tristanjasper commented Aug 21, 2020 •

edited

Loading

tristanjasper Aug 21, 2020

nahumzs Aug 21, 2020

nahumzs commented Aug 21, 2020

nahumzs commented Aug 21, 2020 •

edited by tristanjasper

Loading

tristanjasper commented Aug 24, 2020

nahumzs Aug 24, 2020

tristanjasper Aug 26, 2020

nahumzs Aug 26, 2020

nahumzs Aug 26, 2020

tristanjasper Sep 1, 2020

nahumzs Aug 26, 2020

tristanjasper Aug 26, 2020

alexzherdev commented Aug 26, 2020

tristanjasper commented Aug 26, 2020 •

edited

Loading

tristanjasper commented Aug 27, 2020

tristanjasper Sep 1, 2020

tristanjasper commented Sep 1, 2020

tristanjasper Sep 1, 2020 •

edited

Loading

tristanjasper Sep 1, 2020

nahumzs commented Sep 2, 2020 •

edited

Loading

alexzherdev commented Sep 2, 2020

tristanjasper commented Sep 2, 2020

Ux-157 FormElement api simplification #663

Ux-157 FormElement api simplification #663

Conversation

tristanjasper commented Aug 21, 2020 • edited Loading

Purpose 🚀

Notes ✏️

Updates 📦

Storybook 📕

Screenshots 📸

References 🔗

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahumzs commented Aug 21, 2020

nahumzs commented Aug 21, 2020 • edited by tristanjasper Loading

tristanjasper commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzherdev commented Aug 26, 2020

tristanjasper commented Aug 26, 2020 • edited Loading

tristanjasper commented Aug 27, 2020

Choose a reason for hiding this comment

tristanjasper commented Sep 1, 2020

tristanjasper Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahumzs commented Sep 2, 2020 • edited Loading

alexzherdev commented Sep 2, 2020

tristanjasper commented Sep 2, 2020

tristanjasper commented Aug 21, 2020 •

edited

Loading

nahumzs commented Aug 21, 2020 •

edited by tristanjasper

Loading

tristanjasper commented Aug 26, 2020 •

edited

Loading

tristanjasper Sep 1, 2020 •

edited

Loading

nahumzs commented Sep 2, 2020 •

edited

Loading