Fix remove class in body #85
Conversation
src/react-pure-modal.tsx
Outdated
| @@ -33,10 +33,17 @@ function PureModal(props: Props) { | |||
| const [mouseOffsetY, setMouseOffsetY] = useState(0); | |||
|
|
|||
| const { isOpen, onClose } = props; | |||
| const isClassBody = useMemo(() => document.body.classList.contains('body-modal-fix'), [isOpen]); | |||
There was a problem hiding this comment.
bad idea, you will have expensive layout on each re-render
There was a problem hiding this comment.
Agreed, I've fixed it.
src/react-pure-modal.tsx
Outdated
|
|
||
| useEffect(() => { | ||
| if (isOpen) { | ||
| open(); | ||
| } else if (isClassBody) { | ||
| removeClassBody(); |
There was a problem hiding this comment.
No, you already have it in function unsetModalContext so just check why that function doesn't run when you close the modal
There was a problem hiding this comment.
Correct, but it is not triggered in the code when isOpen changes, so we will invoke it through the effect.
src/react-pure-modal.tsx
Outdated
| @@ -71,7 +66,7 @@ function PureModal(props: Props) { | |||
|
|
|||
| function unsetModalContext() { | |||
| document.removeEventListener('keydown', handleEsc); | |||
| removeClassBody(); | |||
| document.body.classList.remove('body-modal-fix'); | |||
There was a problem hiding this comment.
тобто було гарно одною функцією, а стало тепер інлайново в двох місцях?
There was a problem hiding this comment.
Згоден, не вірно зрозумів спочатку.
src/react-pure-modal.tsx
Outdated
|
|
||
| useEffect(() => { | ||
| if (isOpen) { | ||
| open(); | ||
| } else if (isClassBody) { | ||
| removeClassBody(); | ||
| } else if (document.body.classList.contains('body-modal-fix')) { |
There was a problem hiding this comment.
Тут проблема, що isClassBody має неправильне значення?
There was a problem hiding this comment.
Зрозумів, що useMemo може зробити не вірно при перерендерах кожного разу тай лишні перестворення функції.
|
Скоротив до простого і зрозумілого функціоналу. |
src/react-pure-modal.tsx
Outdated
| useEffect(() => { | ||
| if (isOpen) { | ||
| open(); | ||
| } else { | ||
| unsetModalContext(); |
There was a problem hiding this comment.
check if you can call close function
There was a problem hiding this comment.
You're right, the code does look more logical.
Added dependencies for class removing during 'isOpen' prop changes