-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Fully handle booking pages with conflicting themes being opened together #8921
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -26,7 +26,10 @@ const I18nextAdapter = appWithTranslation<NextJsAppProps<SSRConfig> & { children | |||||
); | ||||||
|
||||||
// Workaround for https://github.com/vercel/next.js/issues/8592 | ||||||
export type AppProps = Omit<NextAppProps<WithNonceProps & Record<string, unknown>>, "Component"> & { | ||||||
export type AppProps = Omit< | ||||||
NextAppProps<WithNonceProps & { themeBasis?: string } & Record<string, unknown>>, | ||||||
"Component" | ||||||
> & { | ||||||
Component: NextAppProps["Component"] & { | ||||||
requiresLicense?: boolean; | ||||||
isThemeSupported?: boolean; | ||||||
|
@@ -72,58 +75,21 @@ const enum ThemeSupport { | |||||
Booking = "userConfigured", | ||||||
} | ||||||
|
||||||
const CalcomThemeProvider = ( | ||||||
props: PropsWithChildren< | ||||||
WithNonceProps & { | ||||||
isBookingPage?: boolean | ((arg: { router: NextRouter }) => boolean); | ||||||
isThemeSupported?: boolean; | ||||||
} | ||||||
> | ||||||
) => { | ||||||
// We now support the inverse of how we handled it in the past. Setting this to false will disable theme. | ||||||
// undefined or true means we use system theme | ||||||
type CalcomThemeProps = PropsWithChildren< | ||||||
Pick<AppProps["pageProps"], "nonce" | "themeBasis"> & | ||||||
Pick<AppProps["Component"], "isBookingPage" | "isThemeSupported"> | ||||||
>; | ||||||
const CalcomThemeProvider = (props: CalcomThemeProps) => { | ||||||
const router = useRouter(); | ||||||
const isBookingPage = (() => { | ||||||
if (typeof props.isBookingPage === "function") { | ||||||
return props.isBookingPage({ router: router }); | ||||||
} | ||||||
|
||||||
return props.isBookingPage; | ||||||
})(); | ||||||
|
||||||
const themeSupport = isBookingPage | ||||||
? ThemeSupport.Booking | ||||||
: // if isThemeSupported is explicitly false, we don't use theme there | ||||||
props.isThemeSupported === false | ||||||
? ThemeSupport.None | ||||||
: ThemeSupport.App; | ||||||
|
||||||
const forcedTheme = themeSupport === ThemeSupport.None ? "light" : undefined; | ||||||
// 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 | ||||||
? `embed-theme-${embedNamespace}` | ||||||
: themeSupport === ThemeSupport.App | ||||||
? "app-theme" | ||||||
: themeSupport === ThemeSupport.Booking | ||||||
? "booking-theme" | ||||||
: undefined; | ||||||
|
||||||
return ( | ||||||
<ThemeProvider | ||||||
nonce={props.nonce} | ||||||
enableColorScheme={false} | ||||||
enableSystem={themeSupport !== ThemeSupport.None} | ||||||
forcedTheme={forcedTheme} | ||||||
storageKey={storageKey} | ||||||
// next-themes doesn't listen to changes on storageKey. So we need to force a re-render when storageKey changes | ||||||
// This is how login to dashboard soft navigation changes theme from light to dark | ||||||
key={storageKey} | ||||||
attribute="class"> | ||||||
<ThemeProvider {...getThemeProviderProps({ props, isEmbedMode, embedNamespace, router })}> | ||||||
{/* Embed Mode can be detected reliably only on client side here as there can be static generated pages as well which can't determine if it's embed mode at backend */} | ||||||
{/* color-scheme makes background:transparent not work in iframe which is required by embed. */} | ||||||
{typeof window !== "undefined" && !isEmbedMode && ( | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* `storageKey` is important because that key is listened for changes(using [`storage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/storage_event) event) and any pages opened will change it's theme based on that(as part of next-themes implementation). | ||||||
* Choosing the right storageKey avoids theme flickering caused by another page using different theme | ||||||
* So, we handle all the cases here namely, | ||||||
* - Both Booking Pages, /free/30min and /pro/30min but configured with different themes but being operated together. | ||||||
* - Embeds using different namespace. They can be completely themed different on the same page. | ||||||
* - Embeds using the same namespace but showing different cal.com links with different themes | ||||||
* - Embeds using the same namespace and showing same cal.com links with different themes(Different theme is possible for same cal.com link in case of embed because of theme config available in embed) | ||||||
* - App has different theme then Booking Pages. | ||||||
* | ||||||
* All the above cases have one thing in common, which is the origin and thus localStorage is shared and thus `storageKey` is critical to avoid theme flickering. | ||||||
* | ||||||
* Some things to note: | ||||||
* - There is a side effect of so many factors in `storageKey` that many localStorage keys will be created if a user goes through all these scenarios(e.g like booking a lot of different users) | ||||||
* - Some might recommend disabling localStorage persistence but that doesn't give good UX as then we would default to light theme always for a few seconds before switching to dark theme(if that's the user's preference). | ||||||
* - We can't disable [`storage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/storage_event) event handling as well because changing theme in one tab won't change the theme without refresh in other tabs. That's again a bad UX | ||||||
* - Theme flickering becomes infinitely ongoing in case of embeds because of the browser's delay in processing `storage` event within iframes. Consider two embeds simulatenously opened with pages A and B. Note the timeline and keep in mind that it happened | ||||||
* because 'setItem(A)' and 'Receives storageEvent(A)' allowed executing setItem(B) in b/w because of the delay. | ||||||
* - t1 -> setItem(A) & Fires storageEvent(A) - On Page A) - Current State(A) | ||||||
* - t2 -> setItem(B) & Fires storageEvent(B) - On Page B) - Current State(B) | ||||||
* - t3 -> Receives storageEvent(A) & thus setItem(A) & thus fires storageEvent(A) (On Page B) - Current State(A) | ||||||
* - t4 -> Receives storageEvent(B) & thus setItem(B) & thus fires storageEvent(B) (On Page A) - Current State(B) | ||||||
* - ... and so on ... | ||||||
*/ | ||||||
function getThemeProviderProps({ | ||||||
props, | ||||||
isEmbedMode, | ||||||
embedNamespace, | ||||||
router, | ||||||
}: { | ||||||
props: Omit<CalcomThemeProps, "children">; | ||||||
isEmbedMode: boolean; | ||||||
embedNamespace: string | null; | ||||||
router: NextRouter; | ||||||
}) { | ||||||
const isBookingPage = (() => { | ||||||
if (typeof props.isBookingPage === "function") { | ||||||
return props.isBookingPage({ router: router }); | ||||||
} | ||||||
return props.isBookingPage; | ||||||
})(); | ||||||
|
||||||
const themeSupport = isBookingPage | ||||||
? ThemeSupport.Booking | ||||||
: // if isThemeSupported is explicitly false, we don't use theme there | ||||||
props.isThemeSupported === false | ||||||
? ThemeSupport.None | ||||||
: ThemeSupport.App; | ||||||
|
||||||
const isBookingPageThemSupportRequired = themeSupport === ThemeSupport.Booking; | ||||||
const themeBasis = props.themeBasis; | ||||||
|
||||||
if ((isBookingPageThemSupportRequired || isEmbedMode) && !themeBasis) { | ||||||
console.warn( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added everywhere, but just in case it's missed, there would be a warning. |
||||||
"`themeBasis` is required for booking page theme support. Not providing it will cause theme flicker." | ||||||
); | ||||||
} | ||||||
|
||||||
const appearanceIdSuffix = themeBasis ? ":" + themeBasis : ""; | ||||||
const forcedTheme = themeSupport === ThemeSupport.None ? "light" : undefined; | ||||||
let embedExplicitlySetThemeSuffix = ""; | ||||||
|
||||||
if (typeof window !== "undefined") { | ||||||
const embedTheme = window.getEmbedTheme(); | ||||||
if (embedTheme) { | ||||||
embedExplicitlySetThemeSuffix = ":" + embedTheme; | ||||||
} | ||||||
} | ||||||
|
||||||
const storageKey = isEmbedMode | ||||||
? // Same Namespace, Same Organizer but different themes would still work seamless and not cause theme flicker | ||||||
// Even though it's recommended to use different namespaces when you want to theme differently on the same page but if the embeds are on different pages, the problem can still arise | ||||||
`embed-theme-${embedNamespace}${appearanceIdSuffix}${embedExplicitlySetThemeSuffix}` | ||||||
: themeSupport === ThemeSupport.App | ||||||
? "app-theme" | ||||||
: isBookingPageThemSupportRequired | ||||||
? `booking-theme${appearanceIdSuffix}` | ||||||
: undefined; | ||||||
|
||||||
return { | ||||||
storageKey, | ||||||
forcedTheme, | ||||||
themeSupport, | ||||||
nonce: props.nonce, | ||||||
enableColorScheme: false, | ||||||
enableSystem: themeSupport !== ThemeSupport.None, | ||||||
// next-themes doesn't listen to changes on storageKey. So we need to force a re-render when storageKey changes | ||||||
// This is how login to dashboard soft navigation changes theme from light to dark | ||||||
key: storageKey, | ||||||
attribute: "class", | ||||||
}; | ||||||
} | ||||||
|
||||||
function FeatureFlagsProvider({ children }: { children: React.ReactNode }) { | ||||||
const flags = useFlags(); | ||||||
return <FeatureProvider value={flags}>{children}</FeatureProvider>; | ||||||
|
@@ -157,6 +217,7 @@ const AppProviders = (props: AppPropsWithChildren) => { | |||||
<TooltipProvider> | ||||||
{/* color-scheme makes background:transparent not work which is required by embed. We need to ensure next-theme adds color-scheme to `body` instead of `html`(https://github.com/pacocoursey/next-themes/blob/main/src/index.tsx#L74). Once that's done we can enable color-scheme support */} | ||||||
<CalcomThemeProvider | ||||||
themeBasis={props.pageProps.themeBasis} | ||||||
nonce={props.pageProps.nonce} | ||||||
isThemeSupported={props.Component.isThemeSupported} | ||||||
isBookingPage={props.Component.isBookingPage}> | ||||||
|
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
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
Abstracted out this theme specific code, which was a lot to a separate fn.