Skip to content
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

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

keithamus
Copy link
Member

@keithamus keithamus commented Nov 9, 2023

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:

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 the cancel or close events on a dialog. This

List the issues that this change affects.

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: 2928786

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

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

@github-actions github-actions bot added javascript Pull requests that update Javascript code ruby Pull requests that update Ruby code css Pull requests that update CSS code labels Nov 9, 2023

This comment was marked as outdated.

@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from 89e11d6 to 21748de Compare November 9, 2023 14:41
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from a39e28c to 407ca81 Compare November 9, 2023 15:58
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from 407ca81 to 3057d3d Compare November 9, 2023 16:00
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from 3c26d94 to 21e06a0 Compare November 15, 2023 10:24
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from c8ac2f5 to 6d58988 Compare November 15, 2023 10:28
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from 78e6cf0 to c681f15 Compare December 6, 2023 12:20
@keithamus keithamus force-pushed the refactor-dialog-to-use-dialog-internally branch from d2f350a to 49f95ba Compare December 6, 2023 14:18
@keithamus keithamus marked this pull request as ready for review December 6, 2023 14:28
@keithamus keithamus requested a review from a team as a code owner December 6, 2023 14:28
@keithamus keithamus requested review from a team, rezrah and camertron December 6, 2023 14:28
@camertron
Copy link
Contributor

One last thing: the screenshots are showing a different text color for the body, is that intentional? Approving for now 👍

@keithamus
Copy link
Member Author

Yeah we should pause on this for a tick because there are some CSS issues that need resolving (e.g. primer/css#2563)

Copy link
Contributor

@langermank langermank left a 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 😄

@keithamus keithamus merged commit 6c45a4b into main Dec 8, 2023
30 checks passed
@keithamus keithamus deleted the refactor-dialog-to-use-dialog-internally branch December 8, 2023 16:42
@primer primer bot mentioned this pull request Dec 8, 2023
@camertron
Copy link
Contributor

Sadly I had to revert this in #2468

@jonrohan jonrohan restored the refactor-dialog-to-use-dialog-internally branch December 20, 2023 18:48
@keithamus keithamus mentioned this pull request Feb 8, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css Pull requests that update CSS code javascript Pull requests that update Javascript code minor release ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants