Fully handle booking pages with conflicting themes being opened together #8921
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request Pull request alert summary
|
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
Current Playwright Test Results Summary✅ 112 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 05/16/2023 07:41:12pm UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: 2599005 Started: 05/16/2023 07:31:51pm UTC
|
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Event Types tests user -- old-booker can add multiple organizer address
Retry 1 • Initial Attempt |
0% (0)0 / 336 runsfailed over last 7 days |
5.06% (17)17 / 336 runsflaked over last 7 days |
📄 apps/web/playwright/booking-seats.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Booking with Seats -- new-booker Reschedule for booking with seats -- old-booker Should reschedule booking with seats and if everyone rescheduled it should be deleted
Retry 1 • Initial Attempt |
0% (0)0 / 349 runsfailed over last 7 days |
77.08% (269)269 / 349 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/action-based.test.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Popup Tests should be able to reschedule
Retry 2 • Retry 1 • Initial Attempt |
10.86% (19)19 / 175 runsfailed over last 7 days |
48.57% (85)85 / 175 runsflaked over last 7 days |
hariombalhara
force-pushed
the
fix/infinite-theme-flickering
branch
from
50de55c
to
5ccc3e1
Compare
CAL-1659 Light Mode-Dark Mode flicker in embed
|
hariombalhara
commented
May 16, 2023
| // Use namespace of embed to ensure same namespaced embed are displayed with same theme. This allows different embeds on the same website to be themed differently | ||
| // One such example is our Embeds Demo and Testing page at http://localhost:3100 | ||
| // Having `getEmbedNamespace` defined on window before react initializes the app, ensures that embedNamespace is available on the first mount and can be used as part of storageKey | ||
| const embedNamespace = typeof window !== "undefined" ? window.getEmbedNamespace() : null; | ||
| const isEmbedMode = typeof embedNamespace === "string"; | ||
|
|
||
| const storageKey = isEmbedMode |
There was a problem hiding this comment.
Abstracted out this theme specific code, which was a lot to a separate fn.
hariombalhara
commented
May 16, 2023
| const appearanceBasis = props.appearanceBasis; | ||
|
|
||
| if ((isBookingPageThemSupportRequired || isEmbedMode) && !appearanceBasis) { | ||
| console.warn( |
There was a problem hiding this comment.
Added everywhere, but just in case it's missed, there would be a warning.
hariombalhara
force-pushed
the
fix/infinite-theme-flickering
branch
from
5ccc3e1
to
ebaf314
Compare
hariombalhara
force-pushed
the
fix/infinite-theme-flickering
branch
from
ebaf314
to
c09828b
Compare
zomars
approved these changes
May 16, 2023
| @@ -140,6 +106,100 @@ const CalcomThemeProvider = ( | |||
| ); | |||
| }; | |||
|
|
|||
| /** | |||
| * The most important job for this fn is to generate correct storageKey for theme persistenc. | |||
There was a problem hiding this comment.
Suggested change
| * The most important job for this fn is to generate correct storageKey for theme persistenc. | |
| * The most important job for this fn is to generate correct storageKey for theme persistence. |
sean-brydon
pushed a commit
that referenced
this pull request
May 18, 2023
…her (#8921) * Handle booking pages with conflicting themes being opened together * Add comments --------- Co-authored-by: Omar López <zomars@me.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Fixes #8860
Fixes CAL-1659
Embed Theme Infinite Flicker when 2 embeds using same namespace try to use different themes simultaneously
Before:
https://www.loom.com/share/717539d93b2f46d5a220b7877f5fdf14
After:
https://www.loom.com/share/85e31e52ee1c4b0781e13e3be380a2ff
Booking Page Theme Switch when interacting with another Booking page (with different theme)
Before:
https://www.loom.com/share/9793f935eff047cf8e57c8c291b49371
After:
https://www.loom.com/share/9731a33ccc3b47969c4622ca15bebeb1
Type of change
How should this be tested?
See the loom
Checklist