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

Add support for initial focus ref to Dialog 2 #4729

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Jul 10, 2024

This reported internally and lack of being able to focus on a specific element when the dialog is open is problematic for some teams. This PR adds support for an initialFocusRef prop to the Dialog component. (As same as Dialog 1, although parity wasn't the motivation.)

Note

We thought about using the native autoFocus attribute however, there is a known bug about using it in React. We also not sure about our timeline of switching to native dialog, we decided to stick with the current implementation. Let me know if you have any concerns 🙌🏻

Changelog

New

  • Add initialFocusRef prop. This prop allows you to specify an element that should receive focus when the dialog opens.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: 2c9eca0

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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 the staff Author is a staff member label Jul 10, 2024
@broccolinisoup broccolinisoup added the skip changeset This change does not need a changelog label Jul 10, 2024
Copy link
Contributor

github-actions bot commented Jul 10, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 91.16 KB (+0.02% 🔺)
packages/react/dist/browser.umd.js 91.52 KB (+0.04% 🔺)

@@ -4,7 +4,7 @@ import {render as HTMLRender, fireEvent} from '@testing-library/react'
import axe from 'axe-core'
import {behavesAsComponent, checkExports} from '../utils/testing'

/* Dialog Version 2 */
/* Dialog Version 1*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you say so 😅😅😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting out of our hand now 🤣 I think version 2 tests are this : https://github.com/primer/react/blob/main/packages/react/src/Dialog/Dialog.test.tsx

Confirmed with looking at the APIs as well - I hope we get it right this time 😅 🤞🏻

@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/332713

@broccolinisoup broccolinisoup removed the skip changeset This change does not need a changelog label Jul 12, 2024
@broccolinisoup broccolinisoup added this pull request to the merge queue Jul 15, 2024
Merged via the queue into main with commit 71bdfa8 Jul 15, 2024
38 checks passed
@broccolinisoup broccolinisoup deleted the initialfocus-dialog2 branch July 15, 2024 02:59
@primer primer bot mentioned this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staff Author is a staff member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants