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

feat(docs): Update modal page to use new layout #713

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

MariaJose
Copy link
Contributor

@MariaJose MariaJose commented Feb 11, 2022

What?

Modal page:

  • Add do and don'ts section in the modal page to use new layout
  • Add keys to fragments to show the corresponding code inside the code editor.

Button page:

  • Update button page to add some <Code primary></Code> tags.

Screenshots/Screen Recordings

Modal page:

Screen Shot 2022-02-11 at 4 04 19 PM

Button page:
Screen Shot 2022-02-11 at 10 58 42 AM

Kapture.2022-02-11.at.11.13.04.mp4

@MariaJose MariaJose requested review from a team as code owners February 11, 2022 17:15
<Code primary>Modals</Code> should require that users take an action.
</>,
<>
<Code primary>Modals</Code> should close when users press the Cancel or Primary button, not when merchants
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if Cancel or Primary button should be like this?:

<>... cancel or primary <NextLink href="/button">Button</NextLink>, ...</>

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just reword the sentence since it doesn't really make sense.
Modals should close when users press an action button, not when merchants click or tap the area outside the Modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

My only quandary with this "Do" is that dialogs should behave differently since a user should be able to click outside to close.

Copy link
Contributor Author

@MariaJose MariaJose Feb 11, 2022

Choose a reason for hiding this comment

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

Sounds good, I'll updated it, makes more sense.

Regarding your comment about the dialog. Noticed that in the dialog example (inside the implementation section), I'm not able to click outside in order to close it. Should I update the prop to closeOnClickOutside={true}?

Also, should we also add this comment in the Do's section? :

When using the variant dialog , users should be able to click outside the dialog to close it by using the closeOnClickOutside prop

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding your comment about the dialog. Noticed that in the dialog example (inside the implementation section), I'm not able to click outside in order to close it. Should I update the prop to closeOnClickOutside={true}?

Probably not, it's really up to the consumer to add that prop.

My only quandary with this "Do" is that dialogs should behave differently since a user should be able to click outside to close.

Probably should have put a 🍹 for this, I was just trying to point out that these Do's and Don'ts were probably written with just the modal variant in mind, where it's more of an experience that we want the user to not escape out of. The dialog variant is really intended for destructive actions, like potentially deleting a product, that we want users to escape out of it easily if it wasn't their intended action. Maybe someone from @bigcommerce/product-design can chip in here?

packages/docs/pages/modal.tsx Show resolved Hide resolved
packages/docs/pages/modal.tsx Show resolved Hide resolved
@MariaJose MariaJose merged commit f489736 into bigcommerce:master Feb 15, 2022
@MariaJose MariaJose deleted the modal-page branch February 15, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants