chore: refactor shell to individual files by sean-brydon · Pull Request #16893 · calcom/cal.diy

sean-brydon · 2024-10-01T10:15:23Z

What does this PR do?

This PR is one of many that refactors the Shell component out of one file and into many sub components. This PR is just extracting with no performance improvements. They will happen in a follow up PR

Mandatory Tasks (DO NOT REMOVE)

I have self-reviewed the code (A decent size PR without self-review might be rejected).
I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

Are there environment variables that should be set?
What are the minimal test data to have?
What is expected (happy path) to have (input and output)?
Any other important info that could help to test that PR

Checklist

I haven't read the contributing guide
My code doesn't follow the style guidelines of this project
I haven't commented my code, particularly in hard-to-understand areas
I haven't checked if my changes generate no new warnings

vercel · 2024-10-01T10:15:32Z

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

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Oct 2, 2024 2:00pm
calcom-web-canary	⬜️ Ignored (Inspect)	Visit Preview		Oct 2, 2024 2:00pm

github-actions · 2024-10-01T10:15:36Z

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Chore/refactor shell". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

sean-brydon · 2024-10-01T12:19:33Z

Move to a hook that can live outside of shell

sean-brydon · 2024-10-01T12:19:47Z

Same goes as above with its helper fuctions

graphite-app · 2024-10-01T12:20:11Z

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (10/01/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (10/01/24)

1 reviewer was added to this PR based on Keith Williams's automation.

sean-brydon · 2024-10-01T12:20:25Z

Create a hook that boots intercom - no need for this to live directly in shell either. Means we can control when the "me" query is called much easier i the future as it was being called on every render

sean-brydon · 2024-10-01T12:20:32Z

Wow thats a lot less lines

sean-brydon · 2024-10-01T12:22:03Z

+  return bannersHeight;
+};
+
+const useBanners = () => {


Lets just have one hook to get all banners and calculate the height

joeauyeung

Great refactor @sean-brydon

Test Cases

Desktop view ✅
Reduced side bar ✅
Mobile nav ❌
Unauthed redirect to login page ✅
Onboarding trigger ✅

Blocking

If we're refactoring right now can we abstract some of the hooks logic so we can prepare for the app router migration? If it's too big then I'm ok having this in a follow up PR. FYI @hbjORbj
In the mobile nav container when clicking More I get this error

Non-blocking

Have you considered using useCallback at all to help with preventing rerenders? I'm also ok with having it in a follow up PR

joeauyeung · 2024-10-01T14:17:36Z

+          onInteractOutside={() => {
+            setMenuOpen(false);
+          }}
+          className="min-w-56 hariom group overflow-hidden rounded-md">


How did @hariombalhara sneak in here?

Thats a great question lol

sean-brydon · 2024-10-02T08:43:48Z

Have you considered using useCallback at all to help with preventing rerenders? I'm also ok with having it in a follow up PR

100%! As i mentioned in the PR description - i've not made any perf improvements here. Just making the component more manageable!

If we're refactoring right now can we abstract some of the hooks logic so we can prepare for the app router migration? If it's too big then I'm ok having this in a follow up PR. FYI @hbjORbj

I don't understand what you mean here? I've tried to abstract all hooks that were present in the shell

Mobile nav ❌
On it! Also some type errors i need to fix around this area too

github-actions · 2024-10-02T09:52:57Z

E2E results are ready!

Workflow #33502.1 latest results

joeauyeung

Navigation is now working as expected.

In terms of my abstracting hooks logic comment. We can't call hooks in server components so ideally I would have liked to see the logic inside of the hooks to be abstracted. Although looking through it some more I realize it would be too big for this PR so let's cross that bridge when we get there.

sean-brydon added 7 commits October 1, 2024 10:40

banners

9dd97a0

useAuthHooks

bf1174a

fixes to redirect and banner

3b7cf17

extract useIntercom to custom hook

d76be7b

use app theme

71dd810

extract user-dropdown to new component

de537c3

Navigation Item

f13c3f4

keithwillcode added consumer core area: core, team members only labels Oct 1, 2024

vercel Bot had a problem deploying to Preview – api October 1, 2024 10:19 Failure

vercel Bot deployed to Preview – dev October 1, 2024 10:30 View deployment

sean-brydon added 2 commits October 1, 2024 11:30

navigation and profile dropdown

902777a

Fix import

09076ab

sean-brydon changed the title ~~Chore/refactor shell~~ chore: refactor shell to individual files Oct 1, 2024

sean-brydon added 2 commits October 1, 2024 13:13

navigation and sidebar

66d0249

Merge branch 'main' into chore/refactor-shell

414690f

sean-brydon marked this pull request as ready for review October 1, 2024 12:17

graphite-app Bot requested a review from a team October 1, 2024 12:17

dosubot Bot added the 💻 refactor label Oct 1, 2024

sean-brydon added the high-risk Requires approval by Foundation team label Oct 1, 2024

graphite-app Bot requested a review from a team October 1, 2024 12:18

sean-brydon commented Oct 1, 2024

View reviewed changes

Comment thread packages/features/shell/Shell.tsx

Copy link
Copy Markdown

Member Author

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

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

Wow thats a lot less lines

sean-brydon commented Oct 1, 2024

View reviewed changes

Comment thread packages/features/shell/banners/LayoutBanner.tsx Outdated

sean-brydon commented Oct 1, 2024

View reviewed changes

vercel Bot had a problem deploying to Preview – api October 1, 2024 12:50 Failure

fix types

9e9f981

vercel Bot had a problem deploying to Preview – api October 1, 2024 13:01 Failure

fix banner prop type

8c38597

vercel Bot had a problem deploying to Preview – api October 1, 2024 13:13 Failure

vercel Bot deployed to Preview – dev October 1, 2024 13:24 View deployment

joeauyeung requested changes Oct 1, 2024

View reviewed changes

Merge branch 'main' into chore/refactor-shell

b080bfe

vercel Bot had a problem deploying to Preview – api October 1, 2024 16:01 Failure

vercel Bot deployed to Preview – dev October 1, 2024 16:11 View deployment

fix mobile nav item import

b9b2eb2

vercel Bot had a problem deploying to Preview – api October 2, 2024 08:58 Failure

fix banner types

38c050c

vercel Bot had a problem deploying to Preview – api October 2, 2024 09:05 Failure

vercel Bot deployed to Preview – dev October 2, 2024 09:26 View deployment

(revert) layout banner render method to fix type error

a7ebeff

vercel Bot deployed to Preview – api October 2, 2024 09:40 View deployment

vercel Bot deployed to Preview – dev October 2, 2024 09:48 View deployment

Merge branch 'main' into chore/refactor-shell

3eb9573

vercel Bot deployed to Preview – api October 2, 2024 11:49 View deployment

vercel Bot deployed to Preview – dev October 2, 2024 11:55 View deployment

sean-brydon requested a review from joeauyeung October 2, 2024 13:04

Merge branch 'main' into chore/refactor-shell

fca3da3

vercel Bot deployed to Preview – api October 2, 2024 14:08 View deployment

joeauyeung approved these changes Oct 2, 2024

View reviewed changes

vercel Bot deployed to Preview – dev October 2, 2024 14:15 View deployment

sean-brydon merged commit 4553455 into main Oct 2, 2024

sean-brydon deleted the chore/refactor-shell branch October 2, 2024 14:35

Conversation

sean-brydon commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

How should this be tested?

Checklist

Uh oh!

vercel Bot commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 1, 2024

Uh oh!

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot commented Oct 1, 2024

Graphite Automations

Uh oh!

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sean-brydon Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

joeauyeung left a comment

Choose a reason for hiding this comment

Test Cases

Blocking

Non-blocking

Uh oh!

joeauyeung Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

sean-brydon Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

sean-brydon commented Oct 2, 2024

Uh oh!

github-actions Bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E results are ready!

Uh oh!

joeauyeung left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sean-brydon commented Oct 1, 2024 •

edited

Loading

vercel Bot commented Oct 1, 2024 •

edited

Loading

github-actions Bot commented Oct 2, 2024 •

edited

Loading