-
Notifications
You must be signed in to change notification settings - Fork 416
fix: clean up classes on glare card dismiss #668
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
base: main
Are you sure you want to change the base?
fix: clean up classes on glare card dismiss #668
Conversation
WalkthroughReplaces local helper-based modal toggling with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GlareCard as GlareCard.vue
participant Vue as Vue Reactivity
participant DOM as Document/Body
rect rgba(220,240,255,0.35)
Note over GlareCard: Component setup
GlareCard->>Vue: defineModel({ default: false })
Vue-->>GlareCard: model = false
GlareCard->>Vue: watch(model, immediate: true)
Vue->>DOM: add/remove `overflow-lock` based on model
end
rect rgba(230,255,230,0.35)
Note over User,GlareCard: User clicks overlay
User->>GlareCard: click overlay
GlareCard->>GlareCard: model = false
Vue->>DOM: remove `overflow-lock`
end
rect rgba(255,235,220,0.35)
Note over GlareCard: Teardown
GlareCard->>Vue: onBeforeUnmount
Vue->>DOM: ensure `overflow-lock` removed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/.vitepress/theme/glare-card/glare-card.vue
(2 hunks)
🔇 Additional comments (3)
docs/.vitepress/theme/glare-card/glare-card.vue (3)
33-33
: LGTM! Direct model assignment correctly triggers cleanup.The change from
setCard(false)
tomodel = false
properly leverages Vue's reactivity to trigger the watch callback that removes the overflow classes.
78-78
: LGTM! Import supports the new watch-based cleanup.
82-82
: LGTM! Default value correctly initializes to closed state.
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.
Actionable comments posted: 1
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/.vitepress/theme/glare-card/glare-card.vue (1)
84-101
: SSR guard resolves the critical issue; consider optional refactor to reduce duplication.The
typeof window === 'undefined'
guards at lines 85 and 97 correctly prevent SSR build errors, resolving the critical concern from the previous review. The logic correctly locks/unlocks overflow when the modal opens/closes and cleans up on unmount.However, the class manipulation logic is duplicated between the watch and unmount hook. Per the previous suggestion, extracting this into a helper function would improve maintainability and allow for a defensive
document.body
check:+const toggleOverflow = (value: boolean) => { + if (typeof window === 'undefined') return + + const { documentElement, body } = document + if (!body) return + + if (value) { + documentElement.classList.add('overflow-hidden') + body.classList.add('overflow-hidden') + } else { + documentElement.classList.remove('overflow-hidden') + body.classList.remove('overflow-hidden') + } +} + -watch(model, (value) => { - if (typeof window === 'undefined') return - - if (value) { - document.documentElement.classList.add('overflow-hidden') - document.body.classList.add('overflow-hidden') - } else { - document.documentElement.classList.remove('overflow-hidden') - document.body.classList.remove('overflow-hidden') - } -}, { immediate: true }) +watch(model, toggleOverflow, { immediate: true }) onBeforeUnmount(() => { - if (typeof window === 'undefined') return - - document.documentElement.classList.remove('overflow-hidden') - document.body.classList.remove('overflow-hidden') + toggleOverflow(false) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/.vitepress/theme/glare-card/glare-card.vue
(2 hunks)
🔇 Additional comments (2)
docs/.vitepress/theme/glare-card/glare-card.vue (2)
33-33
: LGTM!The change from
setCard(false)
tomodel = false
is more idiomatic and directly leverages Vue's reactivity system.
78-82
: LGTM!The imports and model initialization are correct. Using
defineModel({ default: false })
properly establishes the default closed state for the modal.
Previously, after dismissing the glare card (Nice kirakira card BTW! 😉), the
overflow-hidden
onbody
was not cleaned up, which made the page not scrollable. This pull request tries to fix that issue.Summary by CodeRabbit
Bug Fixes
Refactor