V2.0 - Routing Forms and Shell 2.0 everywhere#3902
Conversation
…mprovements/routing-forms
# Conflicts: # apps/web/pages/event-types/[type]/old.tsx # apps/web/pages/event-types/index.tsx
… improvements/routing-forms
# Conflicts: # apps/web/public/static/locales/en/common.json
| const i18n = useViewerI18n(); | ||
|
|
||
| // Don't show any content till translations are loaded. | ||
| // As they are cached infinitely, this status would be loading just once for the app's lifetime until refresh | ||
| if (i18n.status === "loading") { | ||
| return ( | ||
| <div className="absolute z-50 flex h-screen w-full items-center bg-gray-50"> | ||
| <Loader /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (!session && !props.isPublic) return null; |
There was a problem hiding this comment.
Id rather show untranslated string than nothing at all. This will increase the First Contentful Paint and will prevent the empty skeleton static generation. Thoughts? @PeerRich @Jaibles
|
|
||
| function SideBar() { | ||
| const [visible, setVisible] = useState(true); | ||
| const { t } = useLocale(); |
There was a problem hiding this comment.
| const { t } = useLocale(); |
zomars
left a comment
zomars
left a comment
There was a problem hiding this comment.
The counting forms parts LGTM. About Shell tho I have some thoughts.
- Let's try to use and abuse the
getLayoutpattern instead of usingShelleverywhere. - I propose have
ShellandPublicShellorPublicLayoutfor public pages - I don't like the idea of using
LoaderinsideShell, if any we should try to display the loading skeleton so it can be statically generated one build time.
|
Hey @hariombalhara I'm coming in with a little less context so I'm finding it hard to review this PR. Could you add testing notes to the PR description in order to make sure I cover all bases when testing these changes? Also, in the future, do you think it would be possible to break up a PR like this into multiple smaller PRs that are easier to review/test and get to production without worrying about causing breaking changes? |
|
@zlwaterfield I have updated testing notes which should help. wrt breaking this PR down in smaller PRs, I think it was doable but there are a few things to consider and wouldn't have benefitted much. The entire PR can be divided into 2 parts
V2.0 Shell could have been avoided but that would have required me to use old Shell and that would mean that as soon as this PR is merged I would have to immediately switch to using Shell V2 which would have required some rework |
zomars
left a comment
zomars
left a comment
There was a problem hiding this comment.
Removed the Loader so we can merge now. Awesome work @hariombalhara 🙏🏽
5 participants
What does this PR do?
Ignore the number of commits - These are high due to many branches being merged in it.
Loom Demo
Demo Showing Shell is not re-rendered
Fixes #3794 #3795
It also updates Shell to v2 with the following fixes
I took this opportunity to do following as well:
Environment: Production
Type of change
How should this be tested?
Prerequisite: Go and install the Routing Forms App first and then you would see Routing Forms link in Main Navigation
Tests for V2.0 Routing Forms
Tests for V2.0 Shell - Because Routing Forms V2.0 designs are going live with V1, Shell V2.0 deigns has to also go live with V1