[WIP] feat: add dialog component #251
Conversation
|
My vote is for 1 as well. An alert/modal would be for convenience methods anyway, so if we're going to break things I'd rather we break things to make the best possible thing. |
| initialFocus: alert | ||
| ? '.dqpl-dialog-inner' | ||
| : '.dqpl-dialog-show .dqpl-modal-heading', | ||
| allowOutsideClick: () => false |
There was a problem hiding this comment.
We have ClickOutsideListener used elsewhere. It would be nice to converge on a single thing. I think we should evaluate the other places we use ClickOutsideListener to see if they need focus traps and adjust accordingly, maybe as a tech debt thing.
| </button> | ||
| )} | ||
| </div> | ||
| {children} |
There was a problem hiding this comment.
Just being nitpicky here, but do you think this should be wrapped with a class? Alert has dqpl-content so wouldn't we want to possibly style all dialog content as well? I see there's <DialogContent> which I guess lets you opt in, but it feels reductive.
| level: 2 | ||
| }; | ||
| DialogHeading.propTypes = { | ||
| level: PropTypes.number, |
There was a problem hiding this comment.
Consider using PropTypes.oneOf([1,2,3,4,5,6]), since level={500} is invalid.
To address this TODO and move towards removing repeated code, I've gone ahead and created a new
<Dialog />component. The<Modal />and<Alert />components can be replaced by<Dialog />and<Dialog alert />respectively.Moving forward we have a few options that I'd like to discuss...
<Modal />and<Alert />and mark this as aBREAKING CHANGE<Dialog />,<Modal />and<Alert />) and update<Modal />and<Alert />to be simple wrappers which just call<Dialog />under-the-hood<Dialog />a "private" component (as in we don't export it and don't have a demo page for it)fwiw, I strongly prefer option 1 and am happy to discuss why I feel this way
Once we make a decision I will take this PR out of draft / make any necessary updates / write unit tests :)
Reviewer checks
Required fields, to be filled out by PR reviewer(s)