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

Prevent page from scrolling when Modal is open and implement focus trap (#397) #432

Merged
merged 1 commit into from
May 31, 2023
Merged

Prevent page from scrolling when Modal is open and implement focus trap (#397) #432

merged 1 commit into from
May 31, 2023

Conversation

hubacekj
Copy link
Contributor

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

Closes #397

@hubacekj hubacekj changed the title Prevent page from scrolling when is open (#397) Prevent page from scrolling when Modal is open (#397) Nov 22, 2022
@bedrich-schindler
Copy link
Contributor

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

bedrich-schindler
bedrich-schindler reviewed Nov 22, 2022
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
adamkudrna
adamkudrna reviewed Dec 19, 2022
View reviewed changes
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
@hubacekj
Copy link
Contributor Author

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

@hubacekj hubacekj force-pushed the feature/397 branch 2 times, most recently from bf27f58 to 6eafb35 Compare April 11, 2023 12:28
@hubacekj hubacekj mentioned this pull request Apr 14, 2023
Open
@dacerondrej dacerondrej self-requested a review April 14, 2023 13:01
@hubacekj 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
adamkudrna reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

Nicely done! 👏🏻 My major comments are:

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

src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
mbohal
mbohal reviewed May 5, 2023
View reviewed changes
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
@hubacekj 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
Copy link
Contributor Author

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

@hubacekj hubacekj requested review from adamkudrna and mbohal May 15, 2023 11:42
adamkudrna
adamkudrna approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

Great job, thank you! 👏🏻

src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
src/lib/components/Modal/__tests__/Modal.test.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalFocus.js Outdated Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalScrollPrevention.js Show resolved Hide resolved
src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Show resolved Hide resolved
bedrich-schindler
bedrich-schindler requested changes May 18, 2023
View reviewed changes
Copy link
Contributor

@bedrich-schindler bedrich-schindler left a comment

Choose a reason for hiding this comment

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

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

src/lib/components/Modal/_hooks/useModalFocus.js Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalFocus.js Outdated Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalScrollPrevention.js Show resolved Hide resolved
bedrich-schindler
bedrich-schindler requested changes May 18, 2023
View reviewed changes
Copy link
Contributor

@bedrich-schindler bedrich-schindler left a comment

Choose a reason for hiding this comment

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

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
bedrich-schindler requested changes May 18, 2023
View reviewed changes
Copy link
Contributor

@bedrich-schindler bedrich-schindler left a comment

Choose a reason for hiding this comment

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

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

mbohal
mbohal approved these changes May 18, 2023
View reviewed changes
src/lib/components/Modal/Modal.jsx Show resolved Hide resolved
src/lib/components/Modal/Modal.jsx Outdated Show resolved Hide resolved
src/lib/components/Modal/README.mdx Outdated Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalFocus.js Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalFocus.js Outdated Show resolved Hide resolved
src/lib/components/Modal/_hooks/useModalScrollPrevention.js Show resolved Hide resolved
@hubacekj
Copy link
Contributor Author

@bedrich-schindler should be ready for approval

@hubacekj
Copy link
Contributor Author

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.

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

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

adamkudrna
adamkudrna approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

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

@adamkudrna adamkudrna merged commit 0d17b59 into master May 31, 2023
@adamkudrna adamkudrna deleted the feature/397 branch May 31, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbohal mbohal mbohal approved these changes

@adamkudrna adamkudrna adamkudrna approved these changes

@dacerondrej dacerondrej Awaiting requested review from dacerondrej

@bedrich-schindler bedrich-schindler Awaiting requested review from bedrich-schindler bedrich-schindler is a code owner

Assignees

@hubacekj hubacekj

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent page from scrolling when Modal is open
5 participants
@hubacekj @bedrich-schindler @mbohal @adamkudrna @dacerondrej