Fix/Keep themes in peace across embed and booking pages and App by hariombalhara · Pull Request #8108 · calcom/cal.diy

hariombalhara · 2023-04-05T13:20:50Z

What does this PR do?

Fixes #8068

Ensures, different embeds with different namespace can have different themes simultaneously.
Ensure, App can be in one theme for an organizer and his Booking Pages can be in another theme. This is required because App uses System Theme only and Booking Pages use theme configured in Appearance.
Cleans up unnecessary use of useTheme and documents it better so that it's users would know when it should be used.

Environment: Production

Type of change

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

How should this be tested?

Checklist

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

vercel · 2023-04-05T13:20:56Z

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

Name	Status	Preview	Comments	Updated (UTC)
cal	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 17, 2023 11:52am
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 17, 2023 11:52am
web	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 17, 2023 11:52am

hariombalhara · 2023-04-05T13:26:16Z

-  const forcedTheme = !isThemeSupported ? "light" : undefined;
+  const themeSupport = isBookingPage
+    ? ThemeSupport.Booking
+    : props.isThemeSupported === false


if isThemeSupport is explicitly false, then theme would be none otherwise we consider it App Theme

hariombalhara · 2023-04-05T13:26:48Z

+    ? ThemeSupport.None
+    : ThemeSupport.App;
+
+  const forcedTheme = themeSupport === ThemeSupport.None ? "light" : undefined;


If theme is none, we force light mode regardless of what's stored in storage.

github-actions · 2023-04-05T13:36:01Z

📦 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! 🙌

deploysentinel · 2023-04-05T13:39:33Z

Current Playwright Test Results Summary

✅ 67 Passing - ⚠️ 4 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 04/17/2023 12:03:58pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: c4fce55

Started: 04/17/2023 11:57:08am UTC

⚠️ Flakes

📄 apps/web/playwright/webhook.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
add webhook & test that creating an event triggers a webhook call Retry 1 • Initial Attempt	0% (0) 0 / 210 runs failed over last 7 days	1.43% (3) 3 / 210 runs flaked over last 7 days

📄 apps/web/playwright/managed-event-types.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Managed Event Types tests Can create managed event type Retry 1 • Initial Attempt	2.35% (2) 2 / 85 runs failed over last 7 days	35.29% (30) 30 / 85 runs flaked over last 7 days

📄 packages/app-store/routing-forms/playwright/tests/basic.e2e.ts • 2 Flakes

Top 1 Common Error Messages

`null`	2 Test Cases Affected

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Routing Forms Seeded Routing Form Routing Link - Reporting and CSV Download Retry 1 • Initial Attempt	8.33% (17) 17 / 204 runs failed over last 7 days	27.45% (56) 56 / 204 runs flaked over last 7 days
Routing Forms Seeded Routing Form Router URL should work Retry 1 • Initial Attempt	2.45% (5) 5 / 204 runs failed over last 7 days	13.73% (28) 28 / 204 runs flaked over last 7 days

View Detailed Build Results

…ace-across-embed-booking-app

hariombalhara

Self review done

hariombalhara · 2023-04-05T13:31:58Z

  } as const;
 };

+TeamPage.isBookingPage = true;


With new Booker I expect it to be simpler to identify a booking page and thus we can set the prop at one place.

This is required because we can't reliably derive if the page being served is a booking page by it's URL.

hariombalhara · 2023-04-05T13:32:33Z

    />
  );
 }
-FormEditPage.isThemeSupported = true;


We don't need to set isThemeSupported for App Pages. They by default support system theme

hariombalhara · 2023-04-05T13:33:13Z

 }

-RoutingLink.isThemeSupported = true;
+RoutingLink.isBookingPage = true;


For Booking Pages, we don;t need to set isThemeSupported because isBookingPage being true derives that.

hariombalhara · 2023-04-12T05:53:48Z

  const [loading, setLoading] = useState(false);
  const telemetry = useTelemetry();
  const [error, setError] = useState<string | null>(booking ? null : t("booking_already_cancelled"));
-  useTheme(props.theme);


No need to use useTheme here. It's used by booking/[uid] and that has useTheme call.

hariombalhara · 2023-04-12T05:57:45Z

  );
 }

+User.isBookingPage = true;


This has to be set on all booking pages(including routing forms)

I wish there could have been a single place to mark a page as a booking page that can be used by app-providers

cc @JeroenReumkens Not sure if new booker can do this.

hariombalhara · 2023-04-12T06:07:15Z

  --cal-brand-text: white;
 }
 .dark {
-  color-scheme: dark;


@sean-brydon I removed this in the PR. Though it should go live as a separate hotfix once you confirm

github-actions · 2023-04-12T06:16:15Z

📦 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! 🙌

…ace-across-embed-booking-app

github-actions · 2023-04-12T11:32:37Z

📦 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! 🙌

…ace-across-embed-booking-app

sean-brydon

LGTM - One question will this change anything like login/onboarding/password reset where we always force light?

Guessing not since its controlled by isThemeSupported still?

hariombalhara · 2023-04-17T11:27:41Z

isThemeSupported

Yeah isThemeSupported is explicitly false there. So, themeSupport would be none

github-actions · 2023-04-17T11:37:39Z

📦 Next.js Bundle Analysis for @calcom/web

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

Five Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load	% of Budget (`350 KB`)
`/[user]/book`	`188.93 KB`	420.31 KB	120.09% (🟡 +0.14%)
`/apps/[slug]/[...pages]`	`389.06 KB`	620.45 KB	177.27% (🟢 -0.17%)
`/auth/setup`	`81.57 KB`	312.96 KB	89.42% (🟢 -0.17%)
`/d/[link]/book`	`188.58 KB`	419.96 KB	119.99% (🟡 +0.14%)
`/team/[slug]/book`	`188.58 KB`	419.96 KB	119.99% (🟡 +0.14%)

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

github-actions · 2023-04-17T11:56:41Z

📦 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! 🙌

hariombalhara changed the base branch from main to feat/tokens April 5, 2023 13:21

hariombalhara mentioned this pull request Apr 5, 2023

Theme fixes #7870

Closed

7 tasks

hariombalhara commented Apr 5, 2023

View reviewed changes

vercel Bot deployed to Preview – cal April 5, 2023 13:26 View deployment

hariombalhara commented Apr 5, 2023

View reviewed changes

Comment thread apps/web/lib/app-providers.tsx Outdated

hariombalhara force-pushed the fix/make-themes-in-peace-across-embed-booking-app branch from b3be9a9 to 0610a3f Compare April 5, 2023 13:30

vercel Bot deployed to Preview – ui April 5, 2023 13:35 View deployment

vercel Bot deployed to Preview – web April 5, 2023 14:00 View deployment

Base automatically changed from feat/tokens to main April 5, 2023 18:14

Use different storage keys for embed, app and booking pages

2be91a5

hariombalhara force-pushed the fix/make-themes-in-peace-across-embed-booking-app branch from 0610a3f to 2be91a5 Compare April 11, 2023 11:06

vercel Bot deployed to Preview – ui April 11, 2023 11:11 View deployment

vercel Bot deployed to Preview – web April 11, 2023 11:18 View deployment

Merge remote-tracking branch 'origin/main' into fix/make-themes-in-pe…

db77251

…ace-across-embed-booking-app

hariombalhara marked this pull request as ready for review April 12, 2023 05:49

hariombalhara requested a review from a team April 12, 2023 05:49

hariombalhara requested a review from zomars as a code owner April 12, 2023 05:49

hariombalhara requested a review from a team April 12, 2023 05:49

Fix login to dashboard theme switch

86e584b

hariombalhara commented Apr 12, 2023

View reviewed changes

vercel Bot deployed to Preview – ui April 12, 2023 05:55 View deployment

Add comments

c35b868

hariombalhara commented Apr 12, 2023

View reviewed changes

vercel Bot deployed to Preview – web April 12, 2023 06:00 View deployment

Mark dynamic group booking pages

cd98393

hariombalhara commented Apr 12, 2023

View reviewed changes

vercel Bot deployed to Preview – web April 12, 2023 06:10 View deployment

Merge remote-tracking branch 'origin/main' into fix/make-themes-in-pe…

1a14fb4

…ace-across-embed-booking-app

vercel Bot deployed to Preview – ui April 12, 2023 11:25 View deployment

vercel Bot deployed to Preview – web April 12, 2023 11:30 View deployment

Merge remote-tracking branch 'origin/main' into fix/make-themes-in-pe…

703f5be

…ace-across-embed-booking-app

vercel Bot deployed to Preview – ui April 17, 2023 11:00 View deployment

vercel Bot deployed to Preview – cal April 17, 2023 11:00 View deployment

sean-brydon approved these changes Apr 17, 2023

View reviewed changes

hariombalhara requested a review from G3root April 17, 2023 11:25

hariombalhara added the automerge label Apr 17, 2023

hariombalhara removed the request for review from G3root April 17, 2023 11:30

Fix tS

c4fce55

hariombalhara force-pushed the fix/make-themes-in-peace-across-embed-booking-app branch from c961f5b to c4fce55 Compare April 17, 2023 11:44

hariombalhara requested a review from G3root April 17, 2023 11:46

vercel Bot deployed to Preview – cal April 17, 2023 11:52 View deployment

vercel Bot deployed to Preview – ui April 17, 2023 11:52 View deployment

hariombalhara merged commit 2c96444 into main Apr 17, 2023

hariombalhara deleted the fix/make-themes-in-peace-across-embed-booking-app branch April 17, 2023 12:16

PeerRich added the core area: core, team members only label Jul 24, 2023

Conversation

hariombalhara commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

How should this be tested?

Checklist

Uh oh!

vercel Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

deploysentinel Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Test Case Results

Top 1 Common Error Messages

Test Case Results

Uh oh!

hariombalhara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 12, 2023

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

github-actions Bot commented Apr 12, 2023

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

sean-brydon left a comment

Choose a reason for hiding this comment

Uh oh!

hariombalhara commented Apr 17, 2023

Uh oh!

github-actions Bot commented Apr 17, 2023

📦 Next.js Bundle Analysis for @calcom/web

Five Pages Changed Size

Uh oh!

github-actions Bot commented Apr 17, 2023

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hariombalhara commented Apr 5, 2023 •

edited

Loading

vercel Bot commented Apr 5, 2023 •

edited

Loading

github-actions Bot commented Apr 5, 2023 •

edited

Loading

deploysentinel Bot commented Apr 5, 2023 •

edited

Loading