-
Notifications
You must be signed in to change notification settings - Fork 115
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
refactor Dialog to use <dialog> internally #2364
Conversation
🦋 Changeset detectedLatest commit: 2928786 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as outdated.
This comment was marked as outdated.
89e11d6
to
21748de
Compare
a39e28c
to
407ca81
Compare
407ca81
to
3057d3d
Compare
3c26d94
to
21e06a0
Compare
c8ac2f5
to
6d58988
Compare
78e6cf0
to
c681f15
Compare
d2f350a
to
49f95ba
Compare
One last thing: the screenshots are showing a different text color for the body, is that intentional? Approving for now 👍 |
Yeah we should pause on this for a tick because there are some CSS issues that need resolving (e.g. primer/css#2563) |
24b8c02
to
afc1e36
Compare
5d28c88
to
2510ef1
Compare
f3b815a
to
f15f3d0
Compare
f15f3d0
to
2928786
Compare
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 like the visual changes are gone 😄
Sadly I had to revert this in #2468 |
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
This PR removes the
modal-dialog
element we had created to "ponyfill" the native dialog element. Now that native<dialog>
is well supported, we can simply use that and remove the vast majority of our code.We still need a
<dialog-helper>
element for 2 features:dialog-show/close-id
attributes. These will be resolved one day by Invokers (Add commandfor & command attributes to HTMLButtonElement whatwg/html#9841)Screenshots
It is identical in appearance.
Integration
Yes there is a subtle difference which we'll need to change code in dotcom for.
Our
modal-dialog
element mistakenly dispatches thecancel
orclose
events on a dialog. ThisList the issues that this change affects.
Risk Assessment
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.