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.

Sign up for GitHub

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

Jump to bottom

[TrapFocus] Rename TrapFocus to FocusTrap #34216

Merged
merged 22 commits into from Sep 26, 2022
Merged

[TrapFocus] Rename TrapFocus to FocusTrap #34216

merged 22 commits into from Sep 26, 2022

Conversation

kabernardes
Copy link
Contributor

@kabernardes kabernardes commented Sep 7, 2022 •
edited by oliviertassinari

Breaking changes

This PR fixes #33966. As explained in the issue, the component TrapFocus (verb) would be more consistent as FocusTrap (noun).

@mui-bot
Copy link

mui-bot commented Sep 7, 2022 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against a07a6ed

@kabernardes kabernardes marked this pull request as ready for review September 8, 2022 12:56
@kabernardes
Copy link
Contributor Author

Hi, I had a problem with the ci/circleci: test_regressions-1, with exist something that I can do to correct this, please, let me know.

@michaldudak
Copy link
Member

Please merge in/rebase on the latest master. It contains the fix for the regression tests.

@kabernardes
Copy link
Contributor Author

Please merge in/rebase on the latest master. It contains the fix for the regression tests.

Thanks, if you need me to do any changes, just let me know.

@michaldudak michaldudak added the component: FocusTrap The React component. label Sep 13, 2022
michaldudak
michaldudak reviewed Sep 13, 2022
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Good job! One thing left to do is to add a redirect for those who have bookmarked the old docs page.
Please add entries for the docs and API pages in /docs/public/_redirects (somewhere around line 384)

@kabernardes
Copy link
Contributor Author

Good job! One thing left to do is to add a redirect for those who have bookmarked the old docs page. Please add entries for the docs and API pages in /docs/public/_redirects (somewhere around line 384)

I just did that!

@michaldudak
Copy link
Member

I saw you updated the old redirects. That's great, but we also need to introduce new ones pointing from /base/react-trap-focus to /base/react-focus-trap and from /base/api/trap-focus/ to /base/api/focus-trap/

siriwatknp
siriwatknp reviewed Sep 15, 2022
View reviewed changes
test/e2e/index.test.ts
it('should loop the tab key', async () => {
await renderFixture('TrapFocus/OpenTrapFocus');
Copy link
Member

Choose a reason for hiding this comment

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

I guess the fixture/TrapFocus folder should be removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the folder name to fixtures/FocusTrap, I didn't delete and create a new one.

siriwatknp
siriwatknp reviewed Sep 15, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me.

@kabernardes
Copy link
Contributor Author

I saw you updated the old redirects. That's great, but we also need to introduce new ones pointing from /base/react-trap-focus to /base/react-focus-trap and from /base/api/trap-focus/ to /base/api/focus-trap/

Now, I think I did what you ask :)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 15, 2022
@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 Sep 15, 2022
@michaldudak michaldudak added the package: base-ui Specific to @mui/base label Sep 20, 2022
@michaldudak
Copy link
Member

Could you please rebase on the latest master again? I'll do the final review then, and we should be able to merge this in.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 21, 2022
@kabernardes
Copy link
Contributor Author

Could you please rebase on the latest master again? I'll do the final review then, and we should be able to merge this in.

Done!

michaldudak
michaldudak approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for your work!

@siriwatknp please remember to include this PR in MUI Base breaking changes section when doing the next release.

@siriwatknp siriwatknp merged commit c150b7e into mui:master Sep 26, 2022
@michaldudak michaldudak mentioned this pull request Oct 4, 2022
Merged
alexfauquette pushed a commit to alexfauquette/material-ui that referenced this pull request Oct 14, 2022
@kabernardes @alexfauquette
[TrapFocus] Rename TrapFocus to FocusTrap (mui#34216)
0b87e02
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@kabernardes
[TrapFocus] Rename TrapFocus to FocusTrap (mui#34216)
de446a4
oliviertassinari added a commit to mui/mui-x that referenced this pull request Sep 10, 2023
@oliviertassinari
[core] TrapFocus was renamed to FocusTrap
e00af64 
See mui/material-ui#34216.
@oliviertassinari oliviertassinari mentioned this pull request Sep 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siriwatknp siriwatknp siriwatknp left review comments

@michaldudak michaldudak michaldudak approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

@danilo-leal danilo-leal Awaiting requested review from danilo-leal

@m4theushw m4theushw Awaiting requested review from m4theushw

@hbjORbj hbjORbj Awaiting requested review from hbjORbj

Assignees
No one assigned
Labels
breaking change component: FocusTrap The React component. package: base-ui Specific to @mui/base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TrapFocus] Rename to FocusTrap
5 participants
@kabernardes @mui-bot @michaldudak @siriwatknp @oliviertassinari