Skip to content

fix: Avoid duplicate inline embeds#13048

Merged
emrysal merged 1 commit intomainfrom
embed-inline-duplicate-avoid
Jan 5, 2024
Merged

fix: Avoid duplicate inline embeds#13048
emrysal merged 1 commit intomainfrom
embed-inline-duplicate-avoid

Conversation

@hariombalhara
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara commented Jan 5, 2024

What does this PR do?

Fixes #12983

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  • Simply execute inline embed twice. See the issue in production in Codesandbox here

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ai ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:36am
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:36am
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:36am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2024 5:36am
cal-demo ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2024 5:36am
qa ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2024 5:36am
ui ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2024 5:36am

@hariombalhara
Copy link
Copy Markdown
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@hariombalhara hariombalhara changed the title Avoid duplicate inline embeds fix: Avoid duplicate inline embeds Jan 5, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2024

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@keithwillcode keithwillcode added the core area: core, team members only label Jan 5, 2024
elementOrSelector: string | HTMLElement;
config?: PrefillAndIframeAttrsConfig;
}) {
if (this.cal.inlineEl) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Every namespace has his own cal instance, so we just check if the instance has inlineEl already defined.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2024

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Nice, simple fix.

@emrysal emrysal merged commit 6ce6d57 into main Jan 5, 2024
@emrysal emrysal deleted the embed-inline-duplicate-avoid branch January 5, 2024 12:32
@xta
Copy link
Copy Markdown

xta commented Jan 12, 2024

This is breaking my usage of the embed. After opening the embed with a modal, closing the modal, and re-opening the modal, I'm getting the "fix" or error Inline embed already exists. Ignoring this call.

When I look at the DOM, I don't see the embed in my re-opened modal body.

@xta
Copy link
Copy Markdown

xta commented Jan 12, 2024

I was able to work around this, but I think there is a bug. The Cal.instance.inlineEl keeps a reference to the template.content.children[0] even though that part of the DOM no longer exists (since React removed it in my app). https://github.com/calcom/cal.com/blob/main/packages/embeds/embed-core/src/embed.ts#L494C25-L494C53

This prevents the component from creating the iframe in the future when it should be able to.

cc @hariombalhara

@itsfaraaz
Copy link
Copy Markdown

I was able to work around this, but I think there is a bug. The Cal.instance.inlineEl keeps a reference to the template.content.children[0] even though that part of the DOM no longer exists (since React removed it in my app). https://github.com/calcom/cal.com/blob/main/packages/embeds/embed-core/src/embed.ts#L494C25-L494C53

This prevents the component from creating the iframe in the future when it should be able to.

cc @hariombalhara

How'd you work around this? Can't figure this out

@xta
Copy link
Copy Markdown

xta commented Jan 16, 2024

with my setup, I can access global window.Cal.instance.inlineEl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-3366] Avoid duplication of inline embed for same namespace

5 participants