-
Notifications
You must be signed in to change notification settings - Fork 6
Modal #223
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
Modal #223
Conversation
|
Deploying qwik-design-system with
|
Latest commit: |
a5601b1
|
Status: | ✅ Deploy successful! |
Preview URL: | https://045bd102.qwik-design-system.pages.dev |
Branch Preview URL: | https://feat-modal.qwik-design-system.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Modal Scroll Management Issues
The ModalRootBase
component's useTask$
has multiple issues:
- Scroll Management Race Condition: The
cleanup
function re-enables page scroll when the modal opens, immediately before it's disabled, causing flickering or inconsistent scroll behavior. - Delayed Scroll Re-enablement: Page scroll is not immediately re-enabled when the modal closes, as
enablePageScrollFn
is only called in thecleanup
function, which runs later. - Incomplete Initialization/Operation: The
useTask$
only tracksisOpenSig.value
, but scroll setup and modal open/close operations depend oncontentRef.value
. An early return ifcontentRef.value
is undefined can prevent proper initialization or modal functionality.
libs/components/src/modal/modal-root.tsx#L41-L69
qwik-design-system/libs/components/src/modal/modal-root.tsx
Lines 41 to 69 in a5601b1
useTask$(({ track, cleanup }) => { | |
track(() => isOpenSig.value); | |
if (!isMarkedSig.value) { | |
if (!contentRef.value) return; | |
markScrollable(contentRef.value); | |
isMarkedSig.value = true; | |
const { disablePageScroll, enablePageScroll } = createNoScroll({ | |
onInitScrollDisable: initTouchHandler, | |
onResetScrollDisable: resetTouchHandler | |
}); | |
disablePageScrollFn.value = noSerialize(disablePageScroll); | |
enablePageScrollFn.value = noSerialize(enablePageScroll); | |
} | |
if (isOpenSig.value) { | |
contentRef.value?.showModal(); | |
disablePageScrollFn.value?.(); | |
} else { | |
contentRef.value?.close(); | |
} | |
cleanup(() => { | |
enablePageScrollFn.value?.(); | |
}); | |
}); |
Bug: Runtime Dependency Listed Incorrectly
The @fluejs/noscroll
dependency is incorrectly listed in devDependencies
in libs/components/package.json
. As it's imported and used at runtime in modal-root.tsx
, it must be moved to dependencies
to prevent production failures.
libs/components/package.json#L34-L35
qwik-design-system/libs/components/package.json
Lines 34 to 35 in a5601b1
"@builder.io/qwik": "^1.14.1", | |
"@fluejs/noscroll": "^1.0.0", |
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.