Prevent page from scrolling when `Modal` is open and implement focus trap (#397) by hubacekj · Pull Request #432 · react-ui-org/react-ui

hubacekj · 2022-11-22T07:12:53Z

I am not that confident of this implementation. Any suggestions are welcome. @adamkudrna

Closes #397

bedrich-schindler · 2022-11-22T08:30:27Z

Please, fix commit message to: Prevent page from scrolling when Modal is open (#397), you forgot word "Modal"

hubacekj · 2023-04-11T10:04:50Z

Ready for next round, thanks! @adamkudrna @bedrich-schindler @dacerondrej @mbohal

adamkudrna

Nicely done! 👏🏻 My major comments are:

focus trap vs. focus enforce,
code readability because of long hooks.

hubacekj · 2023-05-12T12:43:36Z

Should be ready for hopefully the final round. @bedrich-schindler @adamkudrna

adamkudrna

Great job, thank you! 👏🏻

bedrich-schindler

I would approve it as it is. But there are some opened topics to be resolved. When they are resolved, I will approve it.

bedrich-schindler

I would approve it as it is. But there are some opened topics to be resolved. When they are resolved, I will approve it.

bedrich-schindler

I would approve it as it is. But there are some opened topics to be resolved. When they are resolved, I will approve it.

hubacekj · 2023-05-18T15:29:41Z

@bedrich-schindler should be ready for approval

hubacekj · 2023-05-30T11:46:11Z

Modal now always traps focus. Based on the recent discussion, we leave the option to disable autoFocus for cases in which user doesn't want the long content inside Modal to automatically scroll to the autoFocused element.

bedrich-schindler · 2023-05-30T16:27:29Z

I tested it and it works as expected. I checked code just slightly as I have already seen it. This PR was discussed primarily with @adamkudrna and @mbohal , so I expect them to check it detaily

…trap (#397)

adamkudrna

Cool! I only tweaked a last little detail regarding scrolling and autofocus, and it's ✅.

hubacekj requested review from adamkudrna and mbohal November 22, 2022 07:13

github-actions Bot assigned hubacekj Nov 22, 2022

github-actions Bot added the feature label Nov 22, 2022

hubacekj requested review from bedrich-schindler and dacerondrej November 22, 2022 07:13

hubacekj force-pushed the feature/397 branch from 362a9c2 to 02e62df Compare November 22, 2022 08:11

hubacekj changed the title ~~Prevent page from scrolling when is open (#397)~~ Prevent page from scrolling when Modal is open (#397) Nov 22, 2022

dacerondrej approved these changes Nov 22, 2022

View reviewed changes

bedrich-schindler reviewed Nov 22, 2022

View reviewed changes

Comment thread package-lock.json

Comment thread src/lib/components/Modal/Modal.jsx Outdated

adamkudrna reviewed Dec 19, 2022

View reviewed changes

Comment thread src/lib/components/Modal/Modal.jsx Outdated

Comment thread src/lib/components/Modal/Modal.jsx Outdated

Comment thread src/lib/components/Modal/Modal.jsx Outdated

hubacekj force-pushed the feature/397 branch from 02e62df to 2b0d60b Compare April 3, 2023 12:57

hubacekj requested review from adamkudrna, bedrich-schindler and dacerondrej April 11, 2023 10:05

hubacekj force-pushed the feature/397 branch 2 times, most recently from bf27f58 to 6eafb35 Compare April 11, 2023 12:28

hubacekj mentioned this pull request Apr 14, 2023

Use native <dialog> element for Modal #461

Closed

dacerondrej reviewed Apr 14, 2023

View reviewed changes

Comment thread src/lib/components/Modal/README.mdx

dacerondrej reviewed Apr 14, 2023

View reviewed changes

Comment thread src/lib/components/Modal/README.mdx

dacerondrej self-requested a review April 14, 2023 13:01

hubacekj force-pushed the feature/397 branch from 6eafb35 to 2e02b6e Compare April 24, 2023 09:39

hubacekj commented Apr 24, 2023

View reviewed changes

Comment thread src/lib/components/Modal/Modal.jsx Outdated

hubacekj force-pushed the feature/397 branch from 2e02b6e to c5ae02c Compare April 24, 2023 10:11

hubacekj changed the title ~~Prevent page from scrolling when Modal is open (#397)~~ Prevent page from scrolling when Modal is open and enforce focus (#397) Apr 24, 2023

adamkudrna reviewed Apr 26, 2023

View reviewed changes

mbohal reviewed May 5, 2023

View reviewed changes

Comment thread src/lib/components/Modal/Modal.jsx Outdated

Comment thread src/lib/components/Modal/README.mdx Outdated

hubacekj force-pushed the feature/397 branch from c5ae02c to c536b1a Compare May 12, 2023 11:51

hubacekj force-pushed the feature/397 branch from c536b1a to 3227732 Compare May 12, 2023 12:33

hubacekj changed the title ~~Prevent page from scrolling when Modal is open and enforce focus (#397)~~ Prevent page from scrolling when Modal is open and implement focus trap (#397) May 12, 2023

hubacekj force-pushed the feature/397 branch from 3227732 to 283a78a Compare May 12, 2023 12:42

hubacekj requested review from adamkudrna and mbohal May 15, 2023 11:42

adamkudrna approved these changes May 17, 2023

View reviewed changes

adamkudrna reviewed May 17, 2023

View reviewed changes

Comment thread src/lib/components/Modal/Modal.jsx

bedrich-schindler requested changes May 18, 2023

View reviewed changes

Comment thread src/lib/components/Modal/_hooks/useModalFocus.js

Comment thread src/lib/components/Modal/_hooks/useModalFocus.js Outdated

Comment thread src/lib/components/Modal/_hooks/useModalScrollPrevention.js

bedrich-schindler requested changes May 18, 2023

View reviewed changes

mbohal approved these changes May 18, 2023

View reviewed changes

hubacekj force-pushed the feature/397 branch from 283a78a to 4d53ae6 Compare May 18, 2023 15:28

hubacekj requested a review from bedrich-schindler May 18, 2023 15:29

hubacekj force-pushed the feature/397 branch from 4d53ae6 to c7c6098 Compare May 30, 2023 11:39

hubacekj force-pushed the feature/397 branch 4 times, most recently from 5aaafad to 1119744 Compare May 30, 2023 13:15

Prevent page from scrolling when Modal is open and implement focus …

1f8ab1f

…trap (#397)

adamkudrna force-pushed the feature/397 branch from 1119744 to 1f8ab1f Compare May 31, 2023 10:03

adamkudrna approved these changes May 31, 2023

View reviewed changes

adamkudrna merged commit 0d17b59 into master May 31, 2023

adamkudrna deleted the feature/397 branch May 31, 2023 10:09

Uh oh!

Conversation

hubacekj commented Nov 22, 2022

Uh oh!

bedrich-schindler commented Nov 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hubacekj commented Apr 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamkudrna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hubacekj commented May 12, 2023

Uh oh!

adamkudrna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedrich-schindler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedrich-schindler left a comment

Choose a reason for hiding this comment

Uh oh!

bedrich-schindler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hubacekj commented May 18, 2023

Uh oh!

hubacekj commented May 30, 2023

Uh oh!

bedrich-schindler commented May 30, 2023

Uh oh!

adamkudrna left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants