-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
cc @pmcpinto this PR preps for WP 5.6 release and unbundling of Since our first deploy will be to eComm plan users, they are guaranteed to have the latest, but subsequent steps of the roll out plan may be to users still on 5.5.x which means we'll need to turn off Navigation until they update. Do you think this is a fair requirement for release of the new feature? |
@psealock thanks for the heads up. I think it would be good to add that info to the nav setting description and trigger an inbox notification about that. @elizaan36 what do you think? |
@pmcpinto and I chatted about this today and I think we could include a link to update to 5.5 if the store hasn't already. @psealock Is it possible to present a link to update in the Settings > Advanced > Features area? This would appear for the entire user segment we've released the nav to, regardless of what WordPress version they're on (except for 5.5.X and above.) The checkbox would be disabled until they update, something like this: |
Cool idea, I don't know offhand but bet we could make that work. I misspoke in the original description, Nav elements won't be available until 5.6.0, not 5.5.3. I don't think this changes anything, because only eComm plan will see the Nav but I will need to keep an eye on timing so that we don't offer the Nav and then take it away for users still on 5.5.x. |
6fa8abd
to
261fad3
Compare
Fwiw you need to run @psealock Question: I noticed I could easily get another 30KB of savings by also unbundling wp/compose and wp/date here. Is that worth doing here too? |
Wow 😮 , I did not realize we bundled those! I don't think that was intentional, but more likely to be a result of us not using Dependency Webpack Extraction Plugin and forgetting to declare them as externals. I added them in d6eee46 but that will be taken care of in #5762 which should maybe be combined with this PR because script dependency declarations could cause issues. |
I see the min version of WP for all of WC-Admin is still |
😢 Yikes we did not! Thanks for noticing. Everything seems to be working except for 4cb33b2 provides a workaround if that component is not available. |
I added a Jurassic Ninja site with 5.4 and this branch installed for easy testing to the instructions. cc @jeffstieler |
@psealock I'm testing in the JN site and getting some strange UI issue on the business details page: Seems to be repeated wherever the toggle is used: Everything else looks fine though. |
Thanks for the review @becdetat, this was caused by the styles being bundled still. Instead of bringing in the styles into scss sheets, I enqueued the styles in eee8c97 and made a few fixes in styles. While here, I noticed an issue that is also on NoteI updated the example 5.4 site described in the original issue. Known issueIn 5.4 the ellipsis are off by a bit. I think this is an issue with the Gutenberg styles included in 5.4 because the issue resolves itself in later versions and I can't find wc-admin styles affecting this. |
Seeing as there is a code freeze in early January and there is already one approval, I'm going to merge this so that there are more eyes on this and we have time to spot anything gone wrong. |
Fixes #5034
Bundling
wp.components
has become technical debt and this PR eliminates it from the bundle in favour of the version found on the window.By bumping the minimum version of
Requires at least
to 5.4.0, we are sure that React Hooks are available. This is also in line with Core's minimum version requirements to support current and last two minor versions now that WP 5.6 has been released.This PR automatically turns the Navigation feature flag off for users of WP less than WP 5.6.0 because the
Navigation
component is not available. If Gutenberg feature plugin is active and greater than 9.0.0, then Navigation feature is enabled.This PR does the following
wp.components
Detailed test instructions:
woocommerce_navigation_enabled
to "yes".npm start
. Not doing so could carry over a feature flag config from a previous branch.Consequences
npm ci && npm run analyze
. This isn't as much as previously measured but I'm not sure why.Known Issues and Follow Ups
__experimental
prefix Make usage of__experimantal
prefix safe #5873Changelog Note:
wp.components
and enqueue it instead.