Skip to content

V2.0 Shell - Progressive Rendering with Skeleton#4138

Merged
kodiakhq[bot] merged 8 commits intomainfrom
v2.0/routing-forms-and-other-fixes
Sep 5, 2022
Merged

V2.0 Shell - Progressive Rendering with Skeleton#4138
kodiakhq[bot] merged 8 commits intomainfrom
v2.0/routing-forms-and-other-fixes

Conversation

@hariombalhara
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara commented Sep 3, 2022 •
edited
Loading

What does this PR do?

Fixes # (issue)
Demo Loom
Also Fixes

  • Routing Forms Navigation is not shown as current for Form Builder and Forms list pages
  • Fixes move up and down buttons as per new UI

Screenshot 2022-09-03 at 5 15 11 PM

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link
Copy Markdown

vercel bot commented Sep 3, 2022 •
edited
Loading

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Sep 5, 2022 at 7:10PM (UTC)

@hariombalhara hariombalhara force-pushed the v2.0/routing-forms-and-other-fixes branch from e49a3b9 to fcaac87 Compare September 4, 2022 10:03
@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Sep 5, 2022 •
edited
Loading

this doesnt look good, just remove that skeleton here
CleanShot 2022-09-05 at 10 41 49@2x

(the top two full width ones)

@hariombalhara
Copy link
Copy Markdown
Member Author

hariombalhara commented Sep 5, 2022

@PeerRich Done. Though this will introduce CLS.

@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Sep 5, 2022

@PeerRich Done. Though this will introduce CLS.

I see. maybe instead we wanna have the skeleton as the same size as the content? have one skeleton left for headline and one small one for the button on the right

@hariombalhara
Copy link
Copy Markdown
Member Author

hariombalhara commented Sep 5, 2022

Yeah, that's some extra work just for the skeleton. By default, if no content is there in the element, it just collapses. We need a way to not have the element collapse.
Just thought maybe we can have opacity 0 text in the skeleton element, that would not let it collapse and it won't be visible. This would allow us to just keep using SkeletonText without explicitly hardcoding min-height

@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Sep 5, 2022

Yeah, that's some extra work just for the skeleton. By default, if no content is there in the element, it just collapses. We need a way to not have the element collapse. Just thought maybe we can have opacity 0 text in the skeleton element, that would not let it collapse and it won't be visible. This would allow us to just keep using SkeletonText without explicitly hardcoding min-height

that's a good approach

zomars
zomars reviewed Sep 5, 2022
View reviewed changes
Comment thread apps/web/components/eventtype/CreateEventType.tsx Outdated
@@ -51,7 +52,7 @@ interface CreateEventTypeBtnProps {
export default function CreateEventTypeButton(props: CreateEventTypeBtnProps) {
const { t } = useLocale();
Copy link
Copy Markdown
Contributor

@zomars zomars Sep 5, 2022

Choose a reason for hiding this comment

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

@hariombalhara why not use this instead?

Suggested change
const { t } = useLocale();
const { t, isLocaleReady } = useLocale();

zomars
zomars reviewed Sep 5, 2022
View reviewed changes
Comment thread apps/web/components/I18nLanguageHandler.tsx Outdated
Comment on lines +17 to +21
export function useIsI18nLoading() {
const i18n = useViewerI18n();
return i18n.status === "loading";
}

Copy link
Copy Markdown
Contributor

@zomars zomars Sep 5, 2022

Choose a reason for hiding this comment

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

See my comment

zomars
zomars approved these changes Sep 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Removed isI18nLoading in favor of isLocaleReady, tested and it seems good to go. Thanks @hariombalhara please DM me if you feel this was a bad move.

@zomars zomars added ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Sep 5, 2022
@kodiakhq kodiakhq bot merged commit 52b2ce8 into main Sep 5, 2022
@kodiakhq kodiakhq bot deleted the v2.0/routing-forms-and-other-fixes branch September 5, 2022 19:06
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sean-brydon sean-brydon Awaiting requested review from sean-brydon

1 more reviewer

@zomars zomars zomars approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date core area: core, team members only

Projects

No open projects
Cal.com Core Roadmap
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hariombalhara @PeerRich @zomars