-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
react-dialog: spec #21992
react-dialog: spec #21992
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d092977:
|
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: ee781875d8d21bda2d6ac8267a5b0a3abecb2da9 (build) |
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.
looks great, few questions/comments, thx!
Co-authored-by: André Dias (he/him) <[email protected]>
* | ||
* `alert`: is a special type of modal dialogs that interrupts the user's workflow to communicate an important message or ask for a decision. Unlike a typical modal dialog, the user must take an action through the options given to dismiss the dialog, and it cannot be dismissed through the dimmed background or escape key. | ||
*/ | ||
type?: 'modal' | 'non-modal' | 'alert'; |
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.
As we learnt before it's not the best idea to use the same prop names as native elements with different semantics. Can we consider a different prop name for that?
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.
modalType
perhaps? 🤔, or 3 booleans.... modal
, nonModal
, alert
, WDYT?
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.
No three booleans, please 🐱 Tooltip
uses relationship
, but it obviously does not fit there. modalType
is a bit verbose and kinda confusing as a component named Dialog
...
* closing or toggling a Dialog visibility state. | ||
* @default 'toggle' | ||
*/ | ||
type?: 'open' | 'close' | 'toggle'; |
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.
Do you have a scenario for open
vs toogle
? When open
is needed?
I was thinking about DialogTrigger
with toggle behavior as other components and DialogCloseTrigger
(ugly name).
Again it might not be a good idea to use type
as a prop name there.
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.
So, your suggestion would be a new compound component for the specific case of closing and keeping DialogTrigger
as toggle.
I don't see a huge impact difference in supporting both scenarios in a single component instead though... I think for a moment I'll stick with a single component for this purpose, as they in the end do the same thing: they change open state.
As for the name of the props, how about action
or perhaps 3 boolean properties: open
, close
, toggle
?
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.
action
sounds better than type
indeed.
<!-- ... portal ... --> | ||
``` | ||
|
||
### Async Input submission dialog |
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.
Honestly, this looks like a usage example rather than something that needs to stay in the spec.
Current Behavior
Convergence spec for the Dialog component based on the design spec.
PREVIEW
Related Issue(s)
Fixes partially #20953