-
Notifications
You must be signed in to change notification settings - Fork 345
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
ModalAlert: Add documentation and best practices #2450
ModalAlert: Add documentation and best practices #2450
Conversation
✅ Deploy Preview for gestalt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@@ -1390,6 +1390,56 @@ const GENERAL_COMPONENT_LIST: $ReadOnlyArray<ListItemType> = [ | |||
figma: null, | |||
}, | |||
}, | |||
{ | |||
svg: <Modal />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same svg for now, but will update once I make a new illustration
docs/pages/web/modalalert.js
Outdated
description="Use language that makes it hard to understand what action is being taken, while adding additional actions that may take the user out of their existing context." | ||
cardSize="lg" | ||
type="do" | ||
description="Use to overlay Page content. ModalAlerts should be horizontally and vertically centered on the screen." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one required? I dont think this is specific for ModalAlert but for all Modals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, removed
primaryAction={{ | ||
accessibilityLabel: 'Update credit card info', | ||
label: 'Update credit card', | ||
href: 'https://www.pinterest.com', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we adding the visit icon to all primary actions with URLs? What;s the intention of the icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icon showcases that folks are going to be navigated away from the modal to a new page instead of just closing the modal like non-link buttons
Summary
What changed?
Add documentation and examples for ModalAlert
Why?
Add Best Practice and Variant sandpack examples.
Also updates the padding on Modal and ModalAlert footers
Links
Checklist