chore(calendly): address CodeRabbit follow-ups from #750#758
Merged
Conversation
Three issues raised after the squash-merge of the Calendly PR:
1. **(major) Stylesheet disappears after SPA navigation.** The
`cssInjected` module-level flag short-circuited
`ensureCalendlyStylesheet()` after first injection. But because the
`useHead` call runs inside the composable's `setup()`, the entry is
component-scoped — unhead removes the `<style>` on unmount. On the
next visit the flag still says "already injected", so the widget
loaded unstyled. Drop the flag and rely on `key` deduplication.
2. **(minor) Watch on `status` missed pre-existing error state.** A
shared `useScriptCalendly()` instance that already failed (e.g. on a
prior navigation) won't re-fire `status` change on a new component
mount. Add `{ immediate: true }` so the component emits `error`
when it encounters an already-failed instance.
3. **(minor) `CalendlyPageSettings` missing `hideGdprBanner`.**
Calendly's SDK accepts it; the component's local copy of the
interface had it but the registry's exported one didn't, so callers
composing `CalendlyInlineWidgetOptions` / `CalendlyPopupWidgetOptions`
directly couldn't pass the flag type-safely.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request makes two focused improvements to the Calendly widget integration. First, the status watcher in Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
harlan-zw
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Follow-up to #750.
❓ Type of change
📚 Description
Three issues raised by CodeRabbit on #750 that landed after the squash-merge:
🟠 Major: stylesheet disappears after SPA navigation
The
cssInjectedmodule-level flag short-circuitedensureCalendlyStylesheet()after first injection. But because theuseHeadcall runs inside the composable'ssetup(), the entry is component-scoped — unhead removes the<style>on unmount. On the next visit the flag still said "already injected", so the widget loaded unstyled.Fix: drop the flag and rely on
keydeduplication. Multiple components on the same page callinguseHeadwith the samekeyare safely collapsed to one<style>tag, and the entry follows the component lifecycle correctly across SPA navigations.🟡 Minor:
watchonstatusmissed pre-existing'error'stateA shared
useScriptCalendly()instance that already failed (e.g. on a prior navigation) won't re-firestatuschange on a new component mount. The component never emitserrorfor the existing failure. Added{ immediate: true }.🟡 Minor:
CalendlyPageSettingsmissinghideGdprBannerCalendly's SDK accepts it; the component's local copy of the interface had it but the registry's exported one didn't, so callers composing
CalendlyInlineWidgetOptions/CalendlyPopupWidgetOptionsdirectly couldn't pass the flag type-safely.✅ Verification
/third-parties/calendly/nuxt-scripts, navigate back →<style>was missing); fixed after.