feat(components): Add description to base FormField component #714

joelzwarrington · 2021-10-04T18:37:30Z

Motivations

FormField's do not have a way to display longer information/description outside of an error.

In this PR I've added a description, which acts similarly to the description found in checkbox

Changes

Added

Added description to FormField component (and inputs that inherit)

Changed

Refactored/reorganized FormField tests using Describe so it's easier to follow

Testing

Add description to any input which is inheriting FormField props, such as InputText, InputPassword, Select
Use with Multiline inputs

In Atlantis we use Github's built in pull request reviews.

netlify · 2021-10-04T18:37:35Z

✔️ Deploy Preview for jobber-atlantis ready!

🔨 Explore the source changes: 1ab04e9

🔍 Inspect the deploy log: https://app.netlify.com/sites/jobber-atlantis/deploys/61784d21f04387000702d6ec

😎 Browse the preview: https://deploy-preview-714--jobber-atlantis.netlify.app

chris-at-jobber · 2021-10-04T18:40:17Z

@joelwarrington-jobber can we make sure that this description has a described-by relationship with the input?

Also I was wondering if there's consideration for how this would work if an input also has validation enabled (does the error text replace the description, or stack beneath it?)

adambobadam · 2021-10-04T21:32:53Z

Hey @chris-at-jobber This is what the description field and the validation looks like when they are both present.

adambobadam · 2021-10-04T21:45:44Z

Hey @joelwarrington-jobber would splitting this PR up into two different ones make sense?

There seems to be a big change to the FormField tests and it might be good to see all the changes separately and make sure just the description is passing for now. It would separate the concerns into two, and would be a bit easier to manage.

What do you think?

The description part LGTM though (pending the described-by)!!!

packages/components/src/FormField/FormFieldDescription.tsx

adambobadam · 2021-10-04T22:39:47Z

@chris-at-jobber Should there be a bit of space under the field?

joelzwarrington · 2021-10-04T23:23:56Z

@joelwarrington-jobber can we make sure that this description has a described-by relationship with the input?

Also I was wondering if there's consideration for how this would work if an input also has validation enabled (does the error text replace the description, or stack beneath it?)

@chris-at-jobber Should there be a bit of space under the field?

Going to differ design considerations to @agglasaur as it's needed for some work we're doing

agglasaur · 2021-10-05T15:10:24Z

@joelwarrington-jobber In most cases I think it makes sense for the description to appear with the error field stacked (looking at our usage of similar help text in Jobber Online) - it will be awkward with the password field as the help text is essentially the error as well - im not sure if its worth being able to "replace" it in that instance, or we...slightly reword it so its slightly less awkward?

Otherwise, yes please add the spacing and the described-by

chris-at-jobber · 2021-10-05T15:37:32Z

One visual note - can we either adjust the sizing of the error text to be small, or the description to use base? I would prefer base for both.

…itched to base size

agglasaur · 2021-10-05T20:45:09Z

@chris-at-jobber the checkbox and radio button description field use small by comparison - do you think those should be base?

chris-at-jobber · 2021-10-05T23:10:02Z

Good catch... Those are small so that they're more obviously supplementary to the checkbox/radio label; here I don't think we need to enforce that hierarchy as aggressively as the label is contained in the input. We could go small for both the error and the description, but I don't think there's an issue with the size of the error validation as it stands currently.

chris-at-jobber · 2021-10-06T16:00:17Z

Seeing an unexpected inconsistency in the spacing when InputText is used in isolation, vs in a Form:

Alone:

In a Form:

It looks like the culprit is the Content wrapper, which uses a > selector. When I remove it from the Form example, the description spacing is correct, but the Form collapses in on itself:

I know we have some component-specific reactions to Content in places like Card, Tabs, and Modal - is this another place where we need that, or do we need a wrapper div around the entire InputText + description + error container for this to work?

EDIT: possibly related to this existing open issue?

darryltec · 2021-10-06T18:55:10Z

@chris-at-jobber Not really related to that issue. What's happening here is, like @TJKruitbosch said,

The FormField-implementing components aren't an enclosed single thing

So either

We enclose the FormFieldWrapper with a <Content /> instead of a fragment (</>) and let it handle the spacing between the input, description, and error so we won't need to worry about writing margin css for each element.
Keep those margins but wrap it in a div. AKA replace the fragment with a div

Either way, it needs to be contained by a single element since even if we find a fix for the Content component, it's just a matter of time until someone wraps those fields in a display: flex | grid and we back to square one 😛

joelzwarrington · 2021-10-19T22:34:01Z

@darryltec can you please review this?

darryltec · 2021-10-19T23:03:54Z

Will do first thing tomorrow!

darryltec

Looks good! Sans the input group issue tho.

I would've loved it if the test refactor was done on a different PR as most of it is really unrelated to this whole PR. It could've gone out first too. Basically that unrelated change makes this longer to review.

packages/components/src/FormField/FormField.tsx

packages/components/src/FormField/FormFieldWrapper.tsx

packages/components/src/FormField/FormField.test.tsx

darryltec · 2021-10-21T15:57:26Z

packages/components/src/FormField/FormField.test.tsx

-    </div>
-  `);
-});
+// eslint-disable-next-line max-statements


Any way you can group more things and not have to declare this?

I think the way it's done right now is best and think grouping more provides less value, I really don't like this eslint exception for tests

that's fair!

darryltec · 2021-10-21T16:04:02Z

packages/components/src/FormField/FormFieldWrapper.tsx

@@ -96,8 +104,16 @@ export function FormFieldWrapper({
          <AffixIcon {...suffix} variation="suffix" size={size} />
        )}
      </div>
-      {error && !inline && <InputValidation message={error} />}
-    </>
+      <div>


ah yeah, this will get rendered with no children and the Content will addd a spacing between thus, adding a default margin bottom on all inputs.

In turn, it does this on the input group

resolved in 1ab04e9, decided to go back to the margin top styling instead of using content as it's difficult to get working with the InputGroup

@joelwarrington-jobber Keep in mind that it does bring back these issues

That said

You can also ensure that the empty div doesn't show up

{!inline && (description || error) && ( <div> {description && ( <FormFieldDescription id={descriptionIdentifier} description={description} /> )} {error && <InputValidation message={error} />} </div> )}

Then go to the input group css and update the horizontal code from flex to grid so you can control how it's laid out through the parent class

.horizontal { display: grid; grid-template-columns: repeat(auto-fit, minmax(0, 1fr)); }

This could also be a fast-follow since the spacing issue when used under a <Form /> existed before this PR introduced the description. Description wise, it works!

…de any value

…el' and removed snapshot portion

… snapshot

…ocument instead of being an instance of element

… attribute list

…argin top as to avoid issues with InputGroup

darryltec

The addition of description and test changes looks good to me.

Tho, there are some visual bugs that have been surfaced by QA'ing this that should be fixed on a different PR since it wasn't introduced by this PR. This comment explains how it can be fixed.

Joel Warrington added 5 commits October 4, 2021 12:18

added description type

dcb9037

added component to display form field description

6cc95f3

added description usage to the form field

97e3385

refactored formfield tests

ef8c8ef

added test to find description

68b9e27

joelzwarrington added the design review requested label Oct 4, 2021

joelzwarrington requested a review from agglasaur October 4, 2021 18:37

joelzwarrington self-assigned this Oct 4, 2021

jobbiebot bot added the review requested label Oct 4, 2021

adambobadam reviewed Oct 4, 2021

View reviewed changes

packages/components/src/FormField/FormFieldDescription.tsx Outdated Show resolved Hide resolved

updated htype declaration

43f730f

Joel Warrington added 2 commits October 5, 2021 12:44

added aria-describedby, spacing between description and field, and sw…

dbfb53e

…itched to base size

updated snaps

2f94e11

joelzwarrington requested review from chris-at-jobber and removed request for agglasaur October 6, 2021 15:23

joelzwarrington added the hacktoberfest Some SPOOKY issues for Hacktoberfest. label Oct 7, 2021

Joel Warrington added 2 commits October 7, 2021 09:12

updated how input validation and form are styled and grouped together

139199e

updated snaps

291bfa7

Merge branch 'master' into add-description-field-to-inputs

14a6d09

joelzwarrington requested a review from darryltec October 20, 2021 20:31

darryltec added the in review label Oct 21, 2021

darryltec suggested changes Oct 21, 2021

View reviewed changes

jobbiebot bot added changes requested and removed review requested in review labels Oct 21, 2021

Joel Warrington added 10 commits October 26, 2021 10:27

'descriptionUUID' -> 'descriptionIdentifier'

5b7ba79

'spacing=small' only added to Tag if inline

e22386c

removed 'renders a field with error' snapshot test as it doesnt provi…

2a6cc95

…de any value

'with a placeholder' > 'renders' updated to include 'renders as a lab…

6dbecb8

…el' and removed snapshot portion

'when readonly' > 'renders' updated to get attribute instead of snapshot

de95225

'when disabled' > 'renders' updated to check for attribute instead of…

7c5e5d9

… snapshot

'when loading' > 'should render the spinner' updated to check if in d…

2e706c1

…ocument instead of being an instance of element

'when autocomplete' changed to check for attribute instead of getting…

b784b3f

… attribute list

removed unneeded expect with 'toHaveBeenCalled'

cee37b5

updated how content spacing occurs, instead of using content, using m…

1ab04e9

…argin top as to avoid issues with InputGroup

joelzwarrington requested a review from darryltec October 26, 2021 18:59

jobbiebot bot added the review requested label Oct 26, 2021

joelzwarrington removed the changes requested label Oct 26, 2021

darryltec added the in review label Oct 27, 2021

darryltec approved these changes Oct 27, 2021

View reviewed changes

jobbiebot bot added approved and removed review requested in review labels Oct 27, 2021

joelzwarrington added the hacktoberfest-accepted label Oct 27, 2021

darryltec merged commit 3fae72e into master Oct 29, 2021

darryltec deleted the add-description-field-to-inputs branch October 29, 2021 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(components): Add description to base FormField component #714

feat(components): Add description to base FormField component #714

joelzwarrington commented Oct 4, 2021

netlify bot commented Oct 4, 2021 •

edited

Loading

chris-at-jobber commented Oct 4, 2021

adambobadam commented Oct 4, 2021

adambobadam commented Oct 4, 2021 •

edited

Loading

adambobadam commented Oct 4, 2021

joelzwarrington commented Oct 4, 2021

agglasaur commented Oct 5, 2021

chris-at-jobber commented Oct 5, 2021

agglasaur commented Oct 5, 2021

chris-at-jobber commented Oct 5, 2021

chris-at-jobber commented Oct 6, 2021 •

edited

Loading

darryltec commented Oct 6, 2021 •

edited

Loading

joelzwarrington commented Oct 19, 2021

darryltec commented Oct 19, 2021 •

edited

Loading

darryltec left a comment

darryltec Oct 21, 2021

joelzwarrington Oct 26, 2021

darryltec Oct 27, 2021

darryltec Oct 21, 2021

joelzwarrington Oct 26, 2021

darryltec Oct 27, 2021

darryltec left a comment

feat(components): Add description to base FormField component #714

feat(components): Add description to base FormField component #714

Conversation

joelzwarrington commented Oct 4, 2021

Motivations

Changes

Added

Changed

Testing

netlify bot commented Oct 4, 2021 • edited Loading

chris-at-jobber commented Oct 4, 2021

adambobadam commented Oct 4, 2021

adambobadam commented Oct 4, 2021 • edited Loading

adambobadam commented Oct 4, 2021

joelzwarrington commented Oct 4, 2021

agglasaur commented Oct 5, 2021

chris-at-jobber commented Oct 5, 2021

agglasaur commented Oct 5, 2021

chris-at-jobber commented Oct 5, 2021

chris-at-jobber commented Oct 6, 2021 • edited Loading

Alone:

In a Form:

darryltec commented Oct 6, 2021 • edited Loading

joelzwarrington commented Oct 19, 2021

darryltec commented Oct 19, 2021 • edited Loading

darryltec left a comment

Choose a reason for hiding this comment

darryltec Oct 21, 2021

Choose a reason for hiding this comment

joelzwarrington Oct 26, 2021

Choose a reason for hiding this comment

darryltec Oct 27, 2021

Choose a reason for hiding this comment

darryltec Oct 21, 2021

Choose a reason for hiding this comment

joelzwarrington Oct 26, 2021

Choose a reason for hiding this comment

darryltec Oct 27, 2021

Choose a reason for hiding this comment

That said

darryltec left a comment

Choose a reason for hiding this comment

netlify bot commented Oct 4, 2021 •

edited

Loading

adambobadam commented Oct 4, 2021 •

edited

Loading

chris-at-jobber commented Oct 6, 2021 •

edited

Loading

darryltec commented Oct 6, 2021 •

edited

Loading

darryltec commented Oct 19, 2021 •

edited

Loading