Site Editor: don't hide nav after Gutenberg 12.2 #58830
Conversation
mattwiebe
added
[Status] Needs Review
|
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22790 |
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
mattwiebe
force-pushed
the
update/unhide-site-editor-nav
branch
2 times, most recently
from
5f26445
to
7bf5ff8
Compare
|
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
| @@ -338,6 +338,10 @@ function load_tags_education() { | |||
| * (Core Full Site Editing) | |||
| */ | |||
| function load_wpcom_site_editor() { | |||
| // This is no longer needed after Gutenberg 12.1 due to the Navigation menu no longer being inscrutable. | |||
There was a problem hiding this comment.
Is this comment necessary? I feel that people can always refer to the git blame, check the PR and the explanation on why it is not needed anymore. Comments are often misleading, get outdated pretty quickly, and pollute the source code.
There was a problem hiding this comment.
The point of this code comment is to encourage future maintainers to investigate if this can be removed, should I not get to it. But your point is a fair one, given the number of years-later @todo and @fixme comments strewn about.
There was a problem hiding this comment.
How about an issue instead of a comment? You could even link it as a comment inside the code.
There was a problem hiding this comment.
I think its a handy companion to the version check below. As long as we are conditionally enabling functionality based off a magic gutenberg version number, it seems good to note why for easy reference.
Hopefully it wont get outdated or misleading, as it is accurate to the code beneath it. When that code is updated, then the comment should be as well. Ideally, once we don't support any pre-12.2 Gutenberg version on dotcom anymore we can just remove this entire site editor section of this plugin.
There was a problem hiding this comment.
How about an issue instead of a comment?
An issue sounds good to me in addition, so we can just get rid of all of this in the near future - #59295
There was a problem hiding this comment.
Thanks for creating that issue @Addison-Stavlo ! I intended to do the same but this has been the week of inability to maintain an attention span.
| @@ -338,6 +338,10 @@ function load_tags_education() { | |||
| * (Core Full Site Editing) | |||
| */ | |||
| function load_wpcom_site_editor() { | |||
| // This is no longer needed after Gutenberg 12.1 due to the Navigation menu no longer being inscrutable. | |||
| if ( defined( 'GUTENBERG_VERSION' ) && version_compare( GUTENBERG_VERSION, '12.1.0', '>=' ) ) { | |||
There was a problem hiding this comment.
This should be updated to 12.2.0. I also think that instead of conditional loading we can delete this code and folder after it lands 12.2 lands in production.
There was a problem hiding this comment.
Yup this should all be removed after 12.2.0 hits production. This is simply a mechanism to deploy for edge and production simultaneously.
mattwiebe
changed the title
mattwiebe
force-pushed
the
update/unhide-site-editor-nav
branch
from
ca2c667
to
4a9f6c2
Compare
github-actions
bot
removed
the
[Status] Needs Review
Changes proposed in this Pull Request
Gutenberg 12.2 (ahead of and for WP 5.9) has a new, drastically scaled-back Navigation Sidebar. The
< Dashboardlink is no longer nested-away-by-default, so we can stop hijacking the (W) menu to go back to the Dashboard directly.This also prevents the weird state in which you could open the nav sidebar by picking "browse templates" from the top middle template dropdown without being able to close it again. There was probably an issue about that.
Testing instructions
gutenberg-edgesticker.npm run build:plugin-zipin thegutenberg-core/v12.1.0-rc.1directory on your sandbox to overwrite it. (If you sync a working repo without building, you won't have the requiredGUTENBERG_VERSIONPHP constant.)< Dashboardlink takes you back to Calypso home.