Closed techanvil closed 11 months ago
Sounds good 👍🏻
We might want to tweak the loading state's width/number of elements/etc. but I think this largely sounds good 🙂
IB ✅
Hi @10upsimon!
Ensure that the
<LoadingWrapper>
component matches the height of the<Navigation />
component to avoid a layout shift.
- A solution may be to apply the same class as the nav to the
<LoadingWrapper>
component, i.eclassName="mdc-chip-set googlesitekit-navigation googlesitekit-navigation--mainDashboard"
Unfortunately, just using the same class doesn't make the preview blocks adapt to the height of the navigation bar. The height of the navigation bar is dynamic based on the height of the chips and their quantity. In the proposed PR, I have set it to two fixed heights for mobile & non-mobile viewports. However, this solution isn't perfect as the layout shift would still occur when the chips wrap into two or more lines (as it isn't possible to determine if they'll wrap at the loading stage).
Any thoughts?
CC: @tofumatt
Any thoughts?
@nfmohit I see what you mean, and I agree that it is probably impossible to determine the wrapping final state at the loading state level. In my testing on desktop and mobile, the proposed solution feels more than acceptable, and I do not notice any layout shift, although as you say it may exist in minute proportions.
This LGTM!
Verified:
https://github.com/google/site-kit-wp/assets/73545194/5ce1d700-d32c-4867-8bce-af8d41ca71ec
Feature Description
At present, there is a layout shift on the Dashboard due to the Navigation Bar not being rendered at all while its required state is loading.
Loading:
Loaded:
We should display the Navigation Bar in a loading state to avoid this layout shift.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
DashboardNavigation
entry point file atassets/js/components/DashboardNavigation/index.js
viewableModules === undefined || keyMetrics === undefined
with a<LoadingWrapper>
component that defines the same condition for it'sloading
prop, ie:viewableModules
andkeyMetrics
data has been obtained.<Navigation/>
component is returned as the only child.<LoadingWrapper>
component is 100%.<LoadingWrapper>
component matches the height of the<Navigation />
component to avoid a layout shift.<LoadingWrapper>
component, i.eclassName="mdc-chip-set googlesitekit-navigation googlesitekit-navigation--mainDashboard"
assets/js/components/DashboardNavigation/index.stories.js
<LoadingWrapper>
component.Test Coverage
assets/js/components/DashboardNavigation/index.stories.js
in order to correctly showcase the loading state ofDashboardNavigation
.QA Brief
Changelog entry