Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theme & Docs for Form #1555

Merged
merged 7 commits into from
Aug 25, 2022
Merged

Theme & Docs for Form #1555

merged 7 commits into from
Aug 25, 2022

Conversation

edburyenegren-okta
Copy link
Contributor

@edburyenegren-okta edburyenegren-okta commented Aug 24, 2022

Description

This PR adds documentation, stories, and styles for Form-related components.

The stories are a template for future Form and FieldGroup components. For now, this acts as a guide to self-assembly until those components are available.

TODO

lineHeight: theme.typography.body.lineHeight,
lineHeight: theme.typography.body1.lineHeight,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's body1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body1 is MUI's default for this typography style. We had previously simplified this to body; however, MUI utilizes the style in different places, so I've changed the name back for now.

"& + &": {
".MuiButton-root + &": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could totally break at any point in time. I get that it's not your HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this isn't especially safe. Once we have <ButtonGroup> done, we can remove this sibling styling altogether.

Alternately, I can see if there's a MuiButtonClasses to pull from.

Comment on lines +367 to +369
defaultProps: {
margin: "normal",
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's margin: 'normal'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, margin is a prop and not a CSS Property. There are none, normal, and dense variants in MUI.

Comment on lines +47 to +51
### Length

In general, it is good for forms to be as simple and short as possible. Eliminating unnecessary fields will reduce user effort and translate to better completion rates.

Often in products like Okta, it will be difficult to avoid more complex forms for things like settings or configurations. In these cases based on context, it is an acceptable solution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this means about context.


The `Form` component is designed in a single-column layout to maintain a logical flow of completion.

Adding multiple columns in a form can generate confusion for users and lead to usability problems such as having more than one way to interpret the form and forcing users to reorient their eyes to different places in the form.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like bolding important statements in sentences to make them easier to scan. What do you think?

Suggested change
Adding multiple columns in a form can generate confusion for users and lead to usability problems such as having more than one way to interpret the form and forcing users to reorient their eyes to different places in the form.
Adding multiple columns in a form can generate confusion for users and lead to usability problems such as having more than one way to interpret the form and forcing users to **reorient their eyes** to different places in the form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this and the docs question above, Gretchen and Content Design are going to be reviewing, so I'd rather leave this type of stuff up to them.

Comment on lines +105 to +110
<Box
component="div"
sx={(theme) => ({
marginBottom: theme.spacing(6),
})}
>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to write this using useTheme? I'd like to promote that method in our stories over functions in sx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik this is the best way to access theme variables for <Box>, since it doesn't have default styles or slots. I don't like the style-type application either, though.

I'd like us to get <Form> wrapped/exported asap so that downstream teams aren't doing this themselves.

Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions. Most of them non-blocking.

Approved.

@edburyenegren-okta edburyenegren-okta merged commit cc28933 into develop Aug 25, 2022
@edburyenegren-okta edburyenegren-okta deleted the ee/form branch August 25, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants