fix: Fix AppLayoutToolbar breadcrumbs SSR glitch #3856
Conversation
just-boris
requested review from
just-boris
and removed request for
cansuaa
TrevorBurnham
force-pushed
the
fix-applayouttoolbar-breadcrumbs-ssr-glitch
branch
from
b0f71d9 to
44fb6a1
Compare
|
@just-boris I've updated this PR branch to consolidate duplicated code, and rebased against |
|
There are some failing tests. @TrevorBurnham Can you take a look?
|
just-boris
requested review from
jperals
and removed request for
just-boris
|
@jperals I've updated the branch. Can you re-run the tests and see if they pass now? |
jperals
had a problem deploying
to
dev-pages-react16
GitHub Actions
Failure
jperals
had a problem deploying
to
dev-pages-react18
GitHub Actions
Failure
| contentElAttributes, | ||
| } = skeletonSlotsAttributes; | ||
|
|
||
| const isWidgetLoaded = !!appLayoutState.widgetizedState; |
There was a problem hiding this comment.
I think here we could reuse this isWidgetReady function:
|
|
||
| export function BreadcrumbsSlot({ ownBreadcrumbs, discoveredBreadcrumbs }: BreadcrumbsSlotProps) { | ||
| const isSSR = typeof window === 'undefined'; | ||
| const contextValue = React.useMemo(() => ({ isInToolbar: true }), []); |
There was a problem hiding this comment.
Do we really need to use useMemo here? Unless we really know that the benefit outweighs the overhead, I would be inclined to simplify.
| export function ToolbarBreadcrumbsWrapper({ children, includeTestUtils = false }: ToolbarBreadcrumbsWrapperProps) { | ||
| return ( | ||
| <div | ||
| className={clsx(toolbarStyles['universal-toolbar-breadcrumbs'], includeTestUtils && testutilStyles.breadcrumbs)} |
There was a problem hiding this comment.
Why do we only add the testutilStyles.breadcrumbs class conditionally?
| includeTestUtils?: boolean; | ||
| } | ||
|
|
||
| export function ToolbarBreadcrumbsWrapper({ children, includeTestUtils = false }: ToolbarBreadcrumbsWrapperProps) { |
There was a problem hiding this comment.
This component is only used once further down, so it does not need to be exported, or it could even be removed?
| ); | ||
| } | ||
|
|
||
| export function ToolbarDrawersWrapper() { |
There was a problem hiding this comment.
Also this is used only once below in the same file
| discoveredBreadcrumbs={appLayoutInternals.discoveredBreadcrumbs} | ||
| /> | ||
| </ToolbarSlot> | ||
| <ToolbarSkeletonStructure |
There was a problem hiding this comment.
Codecov points at this line (34) and line 43 below as needing coverage. To be fair, it looks like they were already missing it. Is the change in line 43 necessary though?
| ref={ref as React.Ref<any>} | ||
| className={clsx(styles['toolbar-container'], className)} | ||
| style={{ | ||
| insetBlockStart: style?.insetBlockStart ?? 0, |
There was a problem hiding this comment.
Is this line the actual fix? Undefined was not behaving the same way as 0?
|
I've pushed another commit to try to address the Codecov CI failure. |
Description
When rendering the
AppLayoutToolbarcomponent under the visual refresh theme with SSR, there's a visual glitch where the breadcrumbs are positioned incorrectly before hydration:breadcrumb-glitch.mp4
This PR addresses the issue by making the SSR render more consistent with the client-side render. This is a relatively small change that should have no effect on applications that use client-side rendering only.
There are several other issues with
AppLayoutToolbarin SSR (see #3848); this PR should provide a starting point for fixing them.How has this been tested?
I have a branch where I've set up a local SSR dev server. You can pull that branch down and run
npm run start:watchin one shell andnpm run start:ssrin another.Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.