-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nav Unification - add scaffolding for admin menu experience from static data #45836
Nav Unification - add scaffolding for admin menu experience from static data #45836
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~1522 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Dave for working on this! Works great.
The menu links need some auditing though as commented. Also, it seems that this will clash with the loading status? I would prefer to serve fallbackResponse
on error of the api which is not transparent now. We could then:
a) on initial load, show loading state and fetch menu
b) save the menu to localStorage (either the real or the fallbackResponse
)
c) on subsequent loads, serve the menu from localStorage while waiting for the Api Response to hydrate.
We can surely iterate it though on new PR!
I think I would strongly prefer to never show empty/loading state even in this case:
We know what the menu would roughly look like. I'd rather show 90% of the menu right away and fetch updates from the backend than leave user hanging without the ability to use the main nav. The current situation in Calypso is very similar - it shows as much menu as is common for different sites and configs and in the background it fetches data to determine if it should display some additional items (like Testimonials when enabled).
|
I think that would be great. But I'm not opposed to start with the most common denominator which are the basic wp-admin menus and maybe we can branch them later per site type. Possibly also per wp version, per known list of installed plugins… |
Ok @cpapazoglou @marekhrabe thanks for your input. Very much appreciated. I agree with @marekhrabe that we need to show the most simple nav immediately for uncached page loads. This means we render the nav structure from static default data as shown in this PR. That said, I was also planning on implementing the browser storage approach suggested by @cpapazoglou so I totally agree with that. Please note that once we implement the
This way the only time the user ever sees the static data versoin is on an initial uncached request. Benefits are that user never sees a missing nav. Consider that on slow connections it could take quite a while to retrieve the data from an API endpoint and therefore it's greatly preferable to render something immediately. I appreciate this may make #45831 obsolete, but I still think it is worth having that UI state available as a fallback....just in case. |
3fef9ca
to
e7a453c
Compare
@cpapazoglou Any help you can provide here would be awesome. @obenland The strings are now translatable via |
I think for this stage of development it's fine to use the full menu as a fallback. Later on we'll have to add a capability check to make sure the various user roles get the right menu items |
d6aa894
to
e1825f5
Compare
Outdated - needs new review.
This comment has been minimized.
This comment has been minimized.
const shouldShowLinks = true; | ||
const shouldShowTestimonials = true; | ||
const shouldShowPortfolio = true; | ||
const shouldShowWooCommerce = true; | ||
const shouldShowApperanceHeaderAndBackground = true; | ||
const shouldShowAdControl = true; | ||
const shouldShowAMP = true; | ||
const showShowThemeOptions = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In future I expect that these options will need to be passed into the function as args with defaults specified. This will make the function more testable and avoid building into too many dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getdave Thanks for your great work on this! It looks and works great! I just have an issue that when clicking to a link it reloads the pages instead of loading in place. But let's move forward with this and we can iterate.
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4521234 Thank you @getdave for including a screenshot in the description! This is really helpful for our translators. |
@getdave |
I discussed this with @dlind1 and we agreed to:
|
Translation for this Pull Request has now been finished. |
The aim of this PR is to ensure that users always see a menu appear immediately in the sidebar without any noticeable delay. To do this, if the menu state is not populated we render a basic menu from a static JSON snapshot. Once the REST API request returns we then hydrate the state and update the UI to use the dynamic data.
This is very much an edge case experience, as once the endpoint data is cached in browser storage users will rarely see the fallback experience. Nonetheless this is an important safe guard to ensure a user never receives an empty menu.
Changes proposed in this Pull Request
getAdminMenu
selectoruseSiteMenuItems
hook to return static data from JSON in the event thegetAdminMenu()
selector returns a falsey value due to the state is not yet beingpopulated with menu data from the REST API.Testing instructions
client/state/admin-menu/fallback-data.json
and amend thetitle
prop of the first menu item fromFeedback
toTesting 123
(or similar). Save the file.yarn && yarn start
.Testing 123
(or whatever you modified it to be in the.json
file). Then as the REST API request resolves you should see the Nav update to match the data from the API as generated by D49005-code.Fixes #45487
Screenshots
Notice how the menu changes from
Testing 123
(static fallback data) toFeedback
(dynamic data from REST API).