add form context kind for survey #6529

britt6612 · 2022-12-06T20:32:39Z

What does this PR do?

This PR is a second approach at a Feedback component

This PR includes:
StarRating: This is similar to the previous StarRating in which it is made from RadioButtonGroup and can accept all props
ThumbsRating: This is similar to the previous ThumbsRating in which it is made from RadioButtonGroup and can accept all props

NEW
kind prop was introduced to Form in which formfield uses in order to style the label correctly. If a user passes kind='survey' to form then formfield label will use theme.formfield.survey.label

The 2 main stories are unsolicited and solicited in Form folder on storybook

Where should the reviewer start?

What testing has been done on this PR?

local

How should this be manually tested?

local

Do Jest tests follow these best practices?

TODO:

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

From a discussion with the Grommet maintainers we decided to go with the following in order to simplify the process and make things for flexible in Grommet:

add kind to Form
if FormField is in a Form with certain kind use that kind styling for the FormField
Add theme.formField.feedback to define styling for that kind in the theme (similar to button kinds)

Add `dropButton` with floating prop

Pros:

A user would not have to style a grommet Button inorder for it to be "floating"
Out of the Box with open and close behavior
Less code on the users side as well as less state being updated on their end

Cons:

DropButton is pretty specific in its props with being used with Drop.
per backward compatibility we would need to keep dropAlign dropContent so would it make sense to also add layerContent or layerPosition
The component it self DropButton name is specific to Drop

Add `layerButton` with floating prop

Pros:

A user would not have to style a grommet Button inorder for it to be "floating"
Out of the Box with open and close behavior
Less code on the user's side as well as less state being updated on their end

Cons:

Essentially the code would be very similar to DropButton so not sure it's worth having another component that does the same behavior.
Behavior of the button if the Layer is not interactive is that to opinionated from the Grommet side in how we handle that would be disable the button?

Add `floating` prop to Button

Pros:

A user would not have to style a grommet Button inorder for it to be "floating"

Cons:

the risk of it having it obscure content (which we have been wanting to avoid).
maybe there isn't a lot of cases to have a floating button. (not sure if this is a con though)

Here is the story with a floating Button and Layer just to show to help us decide on what route to go.

Explore LayerButton with floating prop or DropButton with floating prop

What are the relevant issues?

closes grommet/hpe-design-system#3037

Screenshots (if appropriate)

no

Do the grommet docs need to be updated?

yes

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

src/js/components/Form/Form.js

taysea · 2022-12-06T23:16:16Z

src/js/components/FormField/FormField.js

@@ -334,7 +336,10 @@ const FormField = forwardRef(
      borderColor = (themeBorder && themeBorder.color) || 'border';
    }

-    const labelStyle = { ...formFieldTheme.label };
+    let labelStyle;
+    if (formKind === 'survey') {


Do we want kind === 'survey' to be hardcoded or do we want something a bit more loose like how button kind works?

aw okay so if (kind) then do styles rather then === survey

I think it might be nice to have a little more flexibility here like how button handles kinds. This would allow people to have 'marketing' forms or other styling variations we haven't thought about

yes changed this to be whatever kindArg they pass

src/js/components/StarRating/StarRating.js

taysea · 2022-12-06T23:20:34Z

src/js/components/StarRating/StarRating.js

+  scale = 5,
+  value,
+  onChange,
+  outlineColor,


instead of having fillColor and outlineColor, it'd be nice to just have a single color prop. Where color could be:

a string (applies to both fill and outline)

object: { fill: string, outline: string }

Ill also do the same for thumbs

src/js/components/StarRating/StarRating.js

src/js/components/ThumbsRating/ThumbsRating.js

src/js/components/__tests__/__snapshots__/components-test.js.snap

taysea · 2022-12-06T23:26:36Z

src/js/components/__tests__/__snapshots__/components-test.js.snap

+          "survey": {
+            "label": {
+              "margin": {
+                "vertical": "xsmall",


The spacing between question and input and also between formfields looks off -- would be good to double check the desired value.

same comment as above not sure we want these in our base theme or were just going to comment these out and put in hpe theme

I think we should include "survey" in the base grommet theme. It's a reasonable concept that I think would be good to provide.

taysea

Nice progress, I'm liking the kind approach on Form and then just using FormField inside.

taysea · 2022-12-06T23:58:52Z

Between or -- are there any accessibility benefits to using Layer for the feedback vs Drop? I noticed in the DS site we use Layer but we do modal={false}. I don't know the intricacies of accessibility concerns off the top of my head, but if we can implement it the same with Drop then I might be included to use DropButton?

OR

<Button floating onClick={() => setOpen} /> and then just let the user handle their layer as they usually would.

I'm leaning away from Layer button at the moment, pending the accessibility questions I had above.

jcfilben

I like the idea of adding the floating prop to Button. It feels like the other options have more downsides associated with them and may not be routes we want to go. Adding floating to Button seems like a reasonable solution that doesn't add to much extra code or unnecessary work

src/js/components/ThumbsRating/ThumbsRating.js

jcfilben · 2022-12-07T00:02:28Z

src/js/components/FormField/FormField.js

@@ -334,7 +336,10 @@ const FormField = forwardRef(
      borderColor = (themeBorder && themeBorder.color) || 'border';
    }

-    const labelStyle = { ...formFieldTheme.label };
+    let labelStyle;
+    if (formKind === 'survey') {


I think it might be nice to have a little more flexibility here like how button handles kinds. This would allow people to have 'marketing' forms or other styling variations we haven't thought about

britt6612 · 2022-12-07T00:48:26Z

Between or -- are there any accessibility benefits to using Layer for the feedback vs Drop? I noticed in the DS site we use Layer but we do modal={false}. I don't know the intricacies of accessibility concerns off the top of my head, but if we can implement it the same with Drop then I might be included to use DropButton?

OR

<Button floating onClick={() => setOpen} /> and then just let the user handle their layer as they usually would.

I'm leaning away from Layer button at the moment, pending the accessibility questions I had above.

So also with Layer there is an animation that is nice when it opens and closes. Sort of like a slide in which Drop does not have animation Im sure we could add this though as an enhancement.

As far as accessibility Drop does do a better job with having the focus around the whole Drop content then you can down arrow to see what is in the content. Where as Layer we have to add the autoFocus to the X element which is in our DS guidance. However we do not have Drop in our DS system so I wasn't sure how that would look by adding a new component or introducing it and if users would then question layer vs drop

src/js/components/Form/stories/SolicitedFeedback.js

src/js/components/Form/stories/UnsolicitedFeedback.js

src/js/components/FormField/FormField.js

src/js/components/ThumbsRating/index.d.ts

jcfilben · 2022-12-09T20:06:05Z

src/js/components/StarRating/__tests__/StarRating-tests.js

@@ -0,0 +1,45 @@
+import React from 'react';


Can we make this a typescript file? Trying to make sure any new test files that are added are typescript

Looks like this got switched back to a .js file

src/js/components/ThumbsRating/__tests__/ThumbsRating.js

src/js/contexts/FormTypeContext.js

src/js/components/Form/stories/UnsolicitedFeedback.js

ericsoderberghp

Looking good! Just a few more comments.

ericsoderberghp · 2022-12-15T00:40:26Z

src/js/components/Form/stories/SolicitedFeedback.js

+      >
+        <Form
+          onSubmit={({ value }) => console.log('Submit', value)}
+          method="post"


Do we need method="post"?

ericsoderberghp · 2022-12-15T00:40:59Z

src/js/components/Form/stories/SolicitedFeedback.js

+        </Heading>
+      </Header>
+      <Box
+        // Padding used to prevent focus from being cutoff


This seems a bit odd to me. What is cutting the focus off?

src/js/components/Form/stories/SolicitedFeedback.js

ericsoderberghp · 2022-12-15T00:43:41Z

src/js/components/Form/stories/UnsolicitedFeedback.js

+const PositionedFeedbackBox = styled(Box)`
+  bottom: 0px;
+  right: 0px;
+  position: ${(props) => props.theme.feedback?.buttonContainer?.position};


Why does the story need to reach into the theme like this? I would have either expected that to be built in somehow or that those things shouldn't be in the theme at all.

src/js/components/Form/stories/UnsolicitedFeedback.js

ericsoderberghp · 2022-12-15T00:49:29Z

src/js/themes/base.js

@@ -937,6 +937,13 @@ export const generate = (baseSpacing = 24, scale = 6) => {
      },
      margin: { bottom: 'small' },
      // round: undefined,
+      survey: {


I'm OK with the naming being broader than "feedback" on the grommet side. I think it aligns better with what it is for generally.

ericsoderberghp · 2022-12-15T00:49:57Z

src/js/themes/base.js

@@ -1501,6 +1508,18 @@ export const generate = (baseSpacing = 24, scale = 6) => {
      xlarge: { ...fontSizing(2) },
      xxlarge: { ...fontSizing(4) },
    },
+    thumbsRating: {


I'm inclined to not have these colors here as part of the base theme.

ericsoderberghp · 2022-12-15T00:50:16Z

src/js/themes/base.js

@@ -1663,6 +1682,11 @@ export const generate = (baseSpacing = 24, scale = 6) => {
        size: 'medium',
      },
    },
+    starRating: {


I'm inclined to not have this color here as part of the base theme

Co-authored-by: Eric Soderberg <eric.soderberg@hpe.com>

jcfilben

Just a few more comments, I think it is really close

jcfilben · 2022-12-15T21:32:15Z

src/js/components/Form/stories/UnsolicitedFeedback.js

+                method="post"
+                validate="submit"
+                kind="survey"
+                onSubmit={(value) => {


After clicking Submit I am forced to wait until the Layer closes itself. I feel like you should still be able to close the Layer by hitting 'esc' or something so you are not forced to wait

I can add esc thats just the pattern I was following from the DS example

added onClickOutside={onClose}

src/js/components/Form/stories/UnsolicitedFeedback.js

jcfilben · 2022-12-15T21:48:32Z

src/js/components/StarRating/__tests__/StarRating-tests.js

@@ -0,0 +1,45 @@
+import React from 'react';


Looks like this got switched back to a .js file

src/js/components/ThumbsRating/__tests__/ThumbsRating.jsx

jcfilben · 2022-12-15T21:56:19Z

src/js/components/ThumbsRating/ThumbsRating.js

+        return checked ? (
+          <DislikeFill
+            color={
+              theme.thumbsRating?.dislike?.icon?.color ||


I'm fine with the naming that is already here. Maybe we could shorten it to thumbsRating.dislike.color because there is nothing else I would expect to be able to change the color on here besides the icon, so icon is already implied. Soft opinion though, flexible either way.

src/js/components/StarRating/StarRating.js

package.json

…Feedback-component-v2

jcfilben

Looks good!

ericsoderberghp

Just a few.

src/js/components/Form/stories/SolicitedFeedback.js

ericsoderberghp · 2022-12-19T17:22:51Z

src/js/components/Form/stories/SolicitedFeedback.js

+        <FormField label="Comments" htmlFor="comments" name="comments">
+          <TextArea id="comments" name="comments" />
+        </FormField>
+        <Box


Can we use <Footer /> instead of <Box /> here? It feels more semantic.

ericsoderberghp · 2022-12-19T18:23:46Z

src/js/components/StarRating/StarRating.js

+      {(option) =>
+        option <= value ? (
+          <Star
+            color={theme.starRating?.color || theme.starRating?.color?.fill}


This use of theme.starRating.color.fill doesn't align with the typescript definition, where ColorType is just a string or an object with dark/light keys. Can we simplify this to just theme.starRating.color?

aw yes I see your point wondering if I should make theme.starRating.color.fill its own type so we can have theme.starRating.color where someone can pass dark/light keys and also have the theme.starRating.color.fill and theme.starRating.color.outline if they want different colors depending on which icon is selected or you saying for now just allow one color for the icons regardless of fill out outline?

I think the theme should just be asking for a color and we should use it appropriately within the StarRating to apply the color where people would typically expect it, as outline for unselected, as fill for selected.

Co-authored-by: Eric Soderberg <eric.soderberg@hpe.com>

add form context kind for survey

71b0e61

britt6612 requested a review from jcfilben December 6, 2022 20:33

add snapshot

cbee7d0

britt6612 requested a review from taysea December 6, 2022 22:03