-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] add support for more Alert props #2102
Conversation
…tionDuration props
Alert: add canEscapeKeyCancel, canOutsideClickCancel, onClose, transitionDuration propsPreview: documentation | landing | table |
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.
This is awesome! 🎉
packages/core/src/common/errors.ts
Outdated
@@ -9,6 +9,8 @@ const ns = "[Blueprint]"; | |||
export const CLAMP_MIN_MAX = ns + ` clamp: max cannot be less than min`; | |||
|
|||
export const ALERT_WARN_CANCEL_PROPS = ns + ` <Alert> cancelButtonText and onCancel should be set together.`; | |||
export const ALERT_WARN_CANCEL_ESCAPE_KEY = ns + ` <Alert> canEscapeKeyCancel enabled without onCancel handler.`; | |||
export const ALERT_WARN_CANCEL_OUTSIDE_CLICK = ns + ` <Alert> canOutsideClickCancel enbaled without onCancel handler.`; |
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.
These are misleading given onClose
is also ok.
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.
oh yeah gotta update language
/** | ||
* Handler invoked when the Alert is confirmed or canceled; see `onConfirm` and `onCancel` for more details. | ||
* First argument is `true` if confirmed, `false` otherwise. | ||
* This is an alternative to defining separate `onConfirm` and `onCancel` handlers. |
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.
why support 3 handlers when one will do? either remove this (leave the callbacks as-is) or remove the existing ones and only support onClose
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.
TagInput
also has three callbacks, as does EditableText
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.
alright fine 🤷♂️
Update errors.tsPreview: documentation | landing | table |
lintPreview: documentation | landing | table |
Fixes #517
Changes proposed in this pull request:
Alert
: addcanEscapeKeyCancel
,canOutsideClickCancel
,onClose
,transitionDuration
propsonClose
is single alternative to separateonCancel
/onConfirm
propscan*Cancel
propscanEnterKeyConfirm
(see Alert doesn’t have canEscapeKeyClose nor canReturnKeyConfirm #517) because it's not easy to implement (key events require focus, blah blah), and is already supported by simplytab
-ing onto a button and pressingenter
orspace