Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Nav Sidebar: use the official slot API to replace close button #43472

Merged
merged 3 commits into from
Jul 2, 2020
Merged

Nav Sidebar: use the official slot API to replace close button #43472

merged 3 commits into from
Jul 2, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Jun 19, 2020 •
edited
Loading

Note to anyone reading this while preparing a release of the FSE plugin

This change doesn't require any release notes. We're developing a new feature which is currently hidden behind a flag.

Changes proposed in this Pull Request

Related to #43313, this changes the method the nav sidebar is using to override the (W) button in the top-left corner. It replaces the hacky way of overriding the close button with a new API which was added in WordPress/gutenberg#22323.

  • Use <MainDashboardButton> control to override the editors close button
  • Sidebar code now initialised using Gutenberg's registerPlugin() function; no longer need to call React.render() ourselves
  • Remove code that searched for close button using a selector
  • Remove a8c.wpcom-block-editor.shouldCloseEditor hook which is no longer needed
  • wpcom-block-editor detects whether the nav sidebar is active and doesn't override the close button if it's enabled. Otherwise the slot API would add two close buttons.

We have various other pieces of code at a8c that override the close button (e.g. Jetpack, wpcom). We don't need to worry about them interfering with the nav sidebar because using this slot API means the old button is completely removed.

By removing the a8c.wpcom-block-editor.shouldCloseEditor hook the nav sidebar is now entirely independent of wpcom 🎉

Testing instructions

Test on dotorg site

  • Checkout branch
  • cd apps/full-site-editing
  • yarn build or yarn dev
  • Define WPCOM_BLOCK_EDITOR_SIDEBAR in .wp-env.override.json
  • npx wp-env start
  • Test the editor at http://localhost:4013 (username: admin, password: password)
  • Sidebar can be opened/closed via the (W) button. Should work the same as before this PR.

Test on dotcom site

  • Sandbox widgets.wp.com and your favourite
  • Define WPCOM_BLOCK_EDITOR_SIDEBAR in sandbox
  • Apply D45184-code to sandbox
  • Test the editor in your sandboxed site
  • Sidebar can be opened/closed via the (W) button. Should work the same as before this PR.

@p-jackson p-jackson self-assigned this Jun 19, 2020
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/nav-sidebar-button-uses-slot-api

@matticbot
Copy link
Contributor

matticbot commented Jun 19, 2020 •
edited
Loading

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.

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D45184-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@p-jackson p-jackson force-pushed the update/nav-sidebar-button-uses-slot-api branch from 5798280 to f4a60f2 Compare June 21, 2020 22:59
@p-jackson p-jackson changed the base branch from master to update/close-button-override June 21, 2020 23:01
@p-jackson p-jackson force-pushed the update/nav-sidebar-button-uses-slot-api branch 2 times, most recently from b7409aa to 3c39447 Compare June 22, 2020 03:56
@p-jackson p-jackson mentioned this pull request Jun 22, 2020
Merged
@p-jackson p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 22, 2020
@p-jackson p-jackson marked this pull request as ready for review June 22, 2020 07:48
@p-jackson p-jackson requested review from a team as code owners June 22, 2020 07:48
@p-jackson p-jackson force-pushed the update/close-button-override branch from f26bae9 to 5f1cd88 Compare July 1, 2020 08:18
Base automatically changed from update/close-button-override to master July 1, 2020 09:26
@p-jackson p-jackson force-pushed the update/nav-sidebar-button-uses-slot-api branch from 3c39447 to c25b0ec Compare July 2, 2020 03:17
@p-jackson p-jackson requested a review from a team July 2, 2020 04:21
@ramonjd
Copy link
Member

ramonjd commented Jul 2, 2020

Test on dotorg site (had to install Gutenberg 8.4)
Sidebar can be opened/closed via the (W) button. Should work the same as before this PR. ✅

Screen Shot 2020-07-02 at 7 31 51 pm

@ramonjd
Copy link
Member

ramonjd commented Jul 2, 2020

Test on dotcom site
Sidebar can be opened/closed via the (W) button. Should work the same as before this PR. ✅

Screen Shot 2020-07-02 at 7 49 09 pm

ramonjd
ramonjd reviewed Jul 2, 2020
View reviewed changes
...editing-plugin/wpcom-block-editor-nav-sidebar/src/components/toggle-sidebar-button/index.tsx
icon={ wordpress }
iconSize={ 36 }
onClick={ toggleSidebar }
></Button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/> ?

I feel I have contributed something to this code review now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought prettier would have complained about that 🤔

ramonjd
ramonjd approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests well for me. noice!

i can't really provide any constructive feedback on the implementation, but the code looks neat. :)

@p-jackson
Copy link
Member Author

No problem 😆 it's sort of a boring PR to review because it just works like before. Just less hacky.

@p-jackson
Copy link
Member Author

Rebasing to pick up latest FSE work.

p-jackson added 3 commits July 3, 2020 09:39
@p-jackson
Override (W) button using <MainDashboardButton> slot
5b53058
@p-jackson
Remove unused a8c.wpcom-block-editor.shouldCloseEditor filter
873e7cb 
Filter no longer needed to coordinate between calypsoify override and
nav sidebar override. The slot API means they are now totally different
buttons.
@p-jackson
Hide regular close button if nav sidebar is active
ce71022
@p-jackson p-jackson force-pushed the update/nav-sidebar-button-uses-slot-api branch from c25b0ec to ce71022 Compare July 2, 2020 21:39
@p-jackson p-jackson added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 2, 2020
@p-jackson p-jackson merged commit 903f43c into master Jul 2, 2020
@p-jackson p-jackson deleted the update/nav-sidebar-button-uses-slot-api branch July 2, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramonjd ramonjd ramonjd approved these changes

Assignees

@p-jackson p-jackson

Labels
[Goal] Editor Improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p-jackson @matticbot @ramonjd