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

change scroll lock module #28

Merged
merged 2 commits into from
Aug 25, 2022
Merged

change scroll lock module #28

merged 2 commits into from
Aug 25, 2022

Conversation

yuki0410-dev
Copy link

change scroll lock module from disable-scroll to body-scroll-lock.

Change

  • remove disable-scroll
  • add body-scroll-lock
  • Moved Modal Option processing
  • add clean up when unmouted. (in useEffect)

Why

src/index.tsx Outdated

const onOpen = useCallback(() => {
if (preventScroll) {
if (modalRef.current != null) {
Copy link
Member

Choose a reason for hiding this comment

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

!== is better.

Copy link
Author

Choose a reason for hiding this comment

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

@shibe97
Thank you for reviewing.
I used it to mean !== null and !== undefined.
The condition of undefined cannot occur, so it has been fixed.

src/index.tsx Outdated

const onClose = useCallback(() => {
if (preventScroll) {
if (modalRef.current != null) {
Copy link
Member

Choose a reason for hiding this comment

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

!== is better.

@yuki0410-dev yuki0410-dev requested a review from shibe97 August 31, 2021 10:01
yuki0410_ and others added 2 commits September 18, 2021 00:46
- remove  disable-scroll
- add body-scroll-lock
- Moved optional processing
@dc7290
Copy link
Member

dc7290 commented Aug 25, 2022

@yuki0410-dev

Thank you!
I also apologize that it has been quite some time.

LGTM!

@dc7290 dc7290 merged commit 126ca7b into microcmsio:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants