-
Notifications
You must be signed in to change notification settings - Fork 2k
Dashboard v2: Implement WP version settings #103560
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~8584 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~148 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. 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. |
114c38f
to
30f4fc6
Compare
@@ -86,6 +86,26 @@ export const fetchPHPVersion = async ( id: string ): Promise< string | undefined | |||
} ); | |||
}; | |||
|
|||
export const fetchWordPressVersion = async ( siteIdOrSlug: string ): Promise< string > => { | |||
return wpcom.req.get( { | |||
path: `/sites/${ siteIdOrSlug }/hosting/wp-version`, |
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.
I couldn't find any docs to this endpoint. Can you share if you have a link that I missed?
Does it just return latest
or beta
? Do we need these labels (in design spec or if we deem them useful)?
Can we get this info and the WP version from a single place?
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.
There's a whole project for this feature: pesfPa-qt-p2.
Yes, the endpoint just return either latest
or beta
. Also, this is how it looks in dashboard v1:

I'm really hesitant to change the endpoint (it would break v1 or whatnot) or the core design of this feature for now... porting features to v2 and changing the functionalities should be different activities.
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.
porting features to v2 and changing the functionalities should be different activities.
Agreed, but we should track what we can improve and/or simplify. I might be missing context, but I think we should consider getting WP info from a single endpoint (probably remove this one and update the calls of v1).
Additionally, not for this PR, we might want to revisit the design for this and add some help text about the options.
I just realized that @matt-west already provided the full Figma design of this setting, which I don't really conform to 🤦 marking this PR as draft for now... |
30f4fc6
to
6c37b7a
Compare
This PR is ready for a re-review. |
path: 'settings/wordpress', | ||
loader: async ( { params: { siteSlug } } ) => { | ||
const site = await queryClient.ensureQueryData( siteQuery( siteSlug ) ); | ||
if ( canUpdateWordPressVersion( site ) ) { |
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.
@youknowriad @ellatrix not sure if this is a good pattern (business logic in the router file)... but I'm not sure what the alternative is.
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.
Maybe it's fine to call API on component level 🤔
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.
For me this is a good approach, it ensure the loader works properly. What happens if we call the API without the check? Does the endpoint fail?
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.
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.
This also ties back to my suggestion to use beforeLoad
to protect this route. Because if the route was protected using beforeLoad
there won't be any need to do this check here.
I've been thinking and maybe the right approach is going to be this
{
path: 'parent',
beforeLoad: async ({ context }) => {
const sharedData = await fetchSharedData()
return {
context: {
...context,
sharedData,
},
}
},
children: [
{
path: 'child',
beforeLoad: ({ context }) => {
const sharedData = context.sharedData
// use sharedData here
},
}
]
}
- Data that is required to for the "check" is loaded in "beforeLoad" of the parent route and added to the context
- beforeLoad of the child uses that context to "throw" before even "load" is called.
I would love for this to be explored separately as a follow-up.
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.
I don't fully understand, in this case, the parent siteSettingsRoute
would need to fetch everything needed for the setting child routes even though we only open one of them (e.g. WordPress version)?
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.
I don't fully understand, in this case, the parent siteSettingsRoute would need to fetch everything needed for the setting child routes even though we only open one of them (e.g. WordPress version)?
Not really, all we need is the "site" object and the "site" object is already loaded as part of the parent route's loader.
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.
Looks good on my side.
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.
I don’t think we should disable the SummaryButton for simple sites. Let users click through to the setting page where they’ll see the notice about all sites using the latest version of WordPress.
Thanks, makes sense, @matt-west I've updated the PR, please check. |
Looks good. 👍 Thanks @fushar |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17503751 Some locales (Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @fushar for including a screenshot in the description! This is really helpful for our translators. |
Part of DOTCOM-13166
Proposed Changes
This PR implements the WordPress version settings in dashboard v2.
Simple sites
Atomic sites
Staging sites
Testing Instructions
Pre-merge Checklist