[Dialog] Replace deprecated Button for current one

[ItzaMi]

Describe your changes here.

Closes #3062

• replaces deprecated button for the current one
• updates logic for focus (please correct me if I missed something in the use case but what's currently in main doesn't allow to navigate to the 1st button)

Screenshots

Before

before.mp4

After

after.mp4

Merge checklist

• Added/updated tests
• Added/updated documentation
• 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.

[changeset-bot]

🦋 Changeset detected

Latest commit: fd72658

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

[mperrotti]

This seems good to me, but I'd be more comfortable if @broccolinisoup reviews since she's more familiar with this component.

[broccolinisoup]

@ItzaMi 👋🏻 Thank you so much for taking the time to push this PR 🙏🏻 I left some comments. Let me know if you have any concerns/questions 🙌🏻

[broccolinisoup]

I think we should take this back. This manages auto focus when there is a footer button that has autoFocus: true prop.

Story reference, docs reference

[ItzaMi]

I did this change mostly because I saw the following:

• opened the dialog
• pressed TAB once
• no button gets focus
• pressed TAB one more time
• focus is on the 2nd button

Felt a bit weird, don't know if it's intended but also seems out of scope of this PR so I'll rollback this change and can get tackled in a new issue, if that's the case

[broccolinisoup]

Hmm, interesting - I don't see this behaviour. Here is my local. I might be missing something but as you mentioned it is out of this Pr's scope so thank you for taking this back.

Rewatch.Screen.Recording.-.2023-04-20.at.9.38.35.am.mp4

Video description: Dialog storybook page is on the screen and there is a button called "Open Dialog". User tabs into the button and opens the dialog and keep pressing the tab to move the trapped focus on the dialog.

[broccolinisoup]

I also think maybe we shouldn't rename content to children. Because content here is more like a button text content rather than a typical React children prop

[ItzaMi]

I've changed the nomenclature to children because the current Button uses that same prop name ( https://github.com/primer/react/blob/main/src/Button/types.ts#L41 )

[broccolinisoup]

I am mainly concerned because a prop rename will potentially cause breaking changes and also Dialog props are a combined with ButtonProps so it would end up having two children props. https://github.com/primer/react/blob/main/src/Dialog/Dialog.tsx#L23

Same concern for renaming the buttonType option here as well https://github.com/primer/react/pull/3084/files#diff-7c9e4f2540e063c93f543e6eaa44cac7dc3b0e55a8fe81030c9e571f72950a3eR27

Would that be possible to revert these as well? And we can chat about them further if you are interested 😊

[ItzaMi]

Same concern for renaming the buttonType option here as well https://github.com/primer/react/pull/3084/files#diff-7c9e4f2540e063c93f543e6eaa44cac7dc3b0e55a8fe81030c9e571f72950a3eR27

I might be looking at it wrong but I believe that this rename for the buttonType is compliant with the non-deprecated Button ( https://github.com/primer/react/blob/main/src/Button/types.ts#L11 )

Regarding the children vs content, should we then Omit children on this extension of Button? https://github.com/primer/react/pull/3084/files#diff-7c9e4f2540e063c93f543e6eaa44cac7dc3b0e55a8fe81030c9e571f72950a3eR23

Since we're using content for the prop on DialogButtons but Button has a children, there's a slight mismatch, at the moment, since, for example, the footerButtons on the stories, require a content and a children property.

[joshblack]

I wouldn't mind supporting children but definitely wanted to +1 keeping content available during this transition so we don't break any existing usage using content. Do either of you think this could be possible? Something like: {children ?? content} if we end up using both here, and keep content on the DialogButtonProps.

Just an idea, let me know what you think!

[broccolinisoup]

Thanks @joshblack! I love that suggestion! I played with the code a bit today to see how we can support both but the typescript is making trouble and needs children because DialogButtonProps has children as a mandatory prop through Buttons. Is there anything I am missing here? I would really love to support both in the transition period! Is there a way to make Button 's mandatory children prop optional in DialogButtonProps? 😬 Or any other way really.

@ItzaMi you are 💯 right on renaming the buttonType from being normal to default (sorry I missed that earlier) however this is still a breaking change and we really would love to merge this before the major release so following @joshblack's suggestion for supporting both, how about we add default as another option?

/**
   * The type of Button element to use
   */
  buttonType?: 'normal' | 'primary' | 'danger' | 'default'

And on the <Button> element, we could make sure we pass default even if we receive normal

<Button
....
variant={buttonType === 'normal' ? 'default' : buttonType}
...
>

Looking forward to hearing your thoughts! 🙌🏻

[broccolinisoup]

@ItzaMi Sorry for late response! I left comments on the PR, let me know what you think 🙌🏻

[broccolinisoup]

👋🏻 @ItzaMi Are you still happy to follow up with the PR? If not, all good! We are looking into resolving this soon so wanted to check in with you - let us know if you have any questions! Thanks for your work 🙏🏻

[ItzaMi]

👋🏻 @ItzaMi Are you still happy to follow up with the PR? If not, all good! We are looking into resolving this soon so wanted to check in with you - let us know if you have any questions! Thanks for your work 🙏🏻

Hey 👋 Absolute! Just did a commit with the normal changes. Regarding the content / children, how do we stand on that?

[broccolinisoup]

👋🏻 @ItzaMi Are you still happy to follow up with the PR? If not, all good! We are looking into resolving this soon so wanted to check in with you - let us know if you have any questions! Thanks for your work 🙏🏻

Hey 👋 Absolute! Just did a commit with the normal changes. Regarding the content / children, how do we stand on that?

I think we should keep the content as is and omit the children prop.

export type DialogButtonProps = Omit<ButtonProps, 'children'> & {
...

This way type system won't ask children prop and content will be rendered as Button component's children. I tried supporting both as mentioned here, but no luck. Let me know if you give that a try. Thanks!!

[ItzaMi]

👋🏻 @ItzaMi Are you still happy to follow up with the PR? If not, all good! We are looking into resolving this soon so wanted to check in with you - let us know if you have any questions! Thanks for your work 🙏🏻

Hey 👋 Absolute! Just did a commit with the normal changes. Regarding the content / children, how do we stand on that?

I think we should keep the content as is and omit the children prop.

export type DialogButtonProps = Omit<ButtonProps, 'children'> & {
...

This way type system won't ask children prop and content will be rendered as Button component's children. I tried supporting both as mentioned here, but no luck. Let me know if you give that a try. Thanks!!

Should be done now 👍

[broccolinisoup]

@ItzaMi Thanks!! We will also need to;

1. Update the snapshots as there are visual changes. You can do that with npm run test -- -u (reference)
2. Add a changeset

Let me know if you need any help with either of those 🙌🏻 Thanks so much for your contribution 🙏🏻

[ItzaMi]

@broccolinisoup , I've done the required changes, hopefully everything is correct.
There are two snapshot tests that fail on update but I'm unsure on why they actually failed

 FAIL  src/__tests__/exports.test.ts
  ● Test suite failed to run

    Configuration error:

    Could not locate module ./component.module.css mapped as:
    jest-css-modules.

    Please check your configuration for these entries:
    {
      "moduleNameMapper": {
        "/\.css$/": "jest-css-modules"
      },
      "resolver": undefined
    }

      1 | import React from 'react'
      2 | import merge from 'classnames'
    > 3 | import classNames from './component.module.css'
        | ^
      4 |
      5 | export const Component: React.FC<React.HTMLProps<HTMLDivElement>> = ({className, ...props}) => {
      6 |   return <div className={merge(classNames.component, className)} {...props} />

      at createNoMappedModuleFoundError (node_modules/jest-resolve/build/resolver.js:759:17)
      at Object.require (src/drafts/CSSComponent/index.tsx:3:1)
      at Object.require (src/drafts/index.ts:56:1)
      at Object.require (src/__tests__/exports.test.ts:4:1)

 FAIL  src/__tests__/ConfirmationDialog.test.tsx
  ● Test suite failed to run

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'HTMLSpanElement'
        |     property '__reactFiber$pkbppatqhkq' -> object with constructor 'FiberNode'
        --- property 'stateNode' closes the circle
        at stringify (<anonymous>)

      at messageParent (node_modules/jest-runner/node_modules/jest-worker/build/workers/messageParent.js:29:19)

[broccolinisoup]

@ItzaMi Looking at the error on the ConfirmationDialog.test.tsx and see unexpected snapshot changes in the diff, I suspect you might not have the latest changes from the main branch.

If this is the case, could you please make sure your branch has the latest main merged?

After merging the main, please make sure to run consecutively npm install, npm run build and lastly npm run test -- -u to revert back the unexpected snapshots.

Sorry that it is taking longer, and I appreciate your patience with me 🙏🏻

I think after going through the steps above, you might still have some failings in the ConfirmationDialog.test.tsx. We can resolve them by updating the deprecated Button and ActionMenu (doc reference).

Failings are related to the expect(getByText('Primary')).toEqual(document.activeElement) lines. Because we are now using the new Buttons, not the deprecated one, and the DOM structure is different in the new button. You can do expect(getByRole('button', {name: 'Primary'})).toEqual(document.activeElement) instead. Please let me know if you have any failings or need any help. Thanks so much!

[broccolinisoup]

👋🏻 Hi @ItzaMi , so sorry that it took very long but i just pushed commits to fix some misc stuff which is tiny bits. All is looking good now!! Thank you so much for your patience and huge contributions to Primer 🎉 We appreciate it a ton 🙏🏻

The only job is failing now is the changeset job which we are discussing. I'll check back in again in the next few days and merge your PR. Thanks again 🙏🏻