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.

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

(Site Editor)(wpcom-block-editor) Remove dotcom-specific Site Editor "manage all pages" click handler #87347

Merged

Conversation

fullofcaffeine
Copy link
Contributor

@fullofcaffeine fullofcaffeine commented Feb 9, 2024

Proposed Changes

Proposal for fixing #87009.

Related to: #81509, where this override was introduced.

The definitive view should be the new "DataVIew" view from core. I'm not 100% this is the right solution, though, as the removed logic took into account settings that are available only in dotcom, so I'm not sure if we should deprecate it or fix it cc @mmtr

If this is truly deprecated code, then we should probably also look into removing the /wpcom/v2/admin-menu endpoint and its code. Finally, if that's the case, then it seems we could also remove the (related?) https://github.com/Automattic/wp-calypso/blob/trunk/apps/wpcom-block-editor/src/wpcom/features/override-edit-site-back-button.js module? cc @jeyip

Testing Instructions

Considering you're running a dotcom WP simple instance with at least Gutenberg 17.6.2+ installed and active.

  1. Reproduce the issue by following the steps in Gutenberg 17.6.2 RC: Editor >Pages > Manage All Pages redirects to Calypso's /pages #87009. It might depend on the setting returned by /wpcom/v2/admin-menu, but I could reproduce it consistently;
  2. Sandbox widgets.wp.com;
  3. Checkout this branch in In your local wp-calypso copy, then go to apps/wpcom-block-editor and run yarn dev --sync to compile and sync it to your sandbox;
  4. Follow the steps in the issue again and you shouldn't be able to reproduce - it should work as expected.

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 9, 2024
@matticbot
Copy link
Contributor

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.

@jeyip
Copy link
Contributor

jeyip commented Feb 10, 2024

Hi @fullofcaffeine -- I haven't been in touch with the site editor side of Calypso in quite a while, but I don't immediately see any concerns with removing the override I've written

From what I can tell based off of the originating PR, the implications are that the wp-admin site editor's back to dashboard button < Dashboard will lead users back to "My Home" in calypso

Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Thanks @fullofcaffeine, this looks good to me.

look into removing the /wpcom/v2/admin-menu endpoint and its code

That endpoint is still needed, since it powers how we build the left sidebar menu in both Calypso and WP Admin. I just decided to use it for this overriding behavior to determine whether the user prefers the classic view (WP Admin) of the default view (Calypso) for a particular menu item (Pages > All pages).

@fullofcaffeine fullofcaffeine marked this pull request as ready for review February 12, 2024 15:28
@fullofcaffeine
Copy link
Contributor Author

Thanks folks!

@fullofcaffeine fullofcaffeine merged commit b5f2228 into trunk Feb 12, 2024
17 checks passed
@fullofcaffeine fullofcaffeine deleted the remove/deprecated-edit-site-all-pages-override branch February 12, 2024 16:39
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 12, 2024
@fullofcaffeine
Copy link
Contributor Author

Diff: D137905-code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants