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

[Modal] Add support for onKeyDown and remove onEscapeKeyDown #23571

Merged
merged 5 commits into from
Nov 18, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 16, 2020

Breaking changes

  • [Modal] Remove the onEscapeKeyDown prop. It's redundant with the reason argument.
 <Modal
-  onEscapeKeyDown={handleEscapeKeyDown}
+  onClose={(event, reason) => {
+    if (reason === "escapeKeyDown") {
+      handleEscapeKeyDown(event);
+    }
+  }}
 />;

As far as I can tell it can be implemented in userland with onKeyDown or onClose (+ reason).

It was first introduced as onEscapeKeyUp in c644597

@eps1lon eps1lon added breaking change component: modal This is the name of the generic UI component, not the React module! labels Nov 16, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 16, 2020

Details of bundle changes

Generated by 🚫 dangerJS against f372c84

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@eps1lon
Copy link
Member Author

eps1lon commented Nov 16, 2020

There's definitely more opportunity. Some of them have equivalent behavior that we haven't settled on (e.g. preventDefault).

For reviewability and changelog reasons I would work on them separately.

Comment on lines +101 to +107
} else if (
// currently tracking `inProps` which stands for the given props e.g. `function Modal(inProps) {}`
babelTypes.isIdentifier(node, { name: 'inProps' }) &&
// `const props = ...` assuming the right-hand side has `inProps` as input.
babelTypes.isIdentifier(path.node.id, { name: 'props' })
) {
getUsedPropsInternal(path.node.id);
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not pretty since this function has the same issue as most of the code inlined from ttp: Overloading functions to increase re-usability instead of splitting them for individual use cases.

I expect that this kind of props tracking through throughout the component might increase with the removal of withStyles at which point we can refactor this code.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 16, 2020
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 17, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2020
@eps1lon eps1lon merged commit 29fbcce into mui:next Nov 18, 2020
@eps1lon eps1lon deleted the feat/Modal/onKeyDown branch November 18, 2020 15:13
@oliviertassinari
Copy link
Member

Do we miss a description of the breaking change in the migration guide and in the PR's description (I use that to generate the changelog)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: modal This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants