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

Override the backlink on the Live Preview #81387

Closed
wants to merge 2 commits into from

Conversation

okmttdhr
Copy link
Member

@okmttdhr okmttdhr commented Sep 5, 2023

Related to
#80595
WordPress/gutenberg#54174

Proposed Changes

Override the Back on the Live Preview with the 'core/edit-site' store. This PR solves #80595 and works after WordPress/gutenberg#54174.

Testing Instructions

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

@matticbot
Copy link
Contributor

This PR modifies the release build for wpcom-block-editor

To test your changes on WordPress.com, run install-plugin.sh wpcom-block-editor add/themePreviewBackLink on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-l4k-p2

@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.

@arthur791004
Copy link
Contributor

arthur791004 commented Sep 5, 2023

We have to do it on Jetpack. See the site_editor_dashboard_link function and the block_editor_settings_all filter on atomic and wpcom

@okmttdhr
Copy link
Member Author

okmttdhr commented Sep 5, 2023

@arthur791004
I'm a little confused because there are a few places in wpcom-block-editor where we're making changes to __experimentalDashboardLink.

const dashboardLink = select( 'core/edit-site' )?.getSettings?.().__experimentalDashboardLink;

const backButtonUrl = siteEditor.getSettings().__experimentalDashboardLink;

Maybe we should move these into Jetpack?

@arthur791004
Copy link
Contributor

wp-calypso/apps/wpcom-block-editor/src/calypso/features/iframe-bridge-server.js

I guess we might not need this for the site editor since we have un-iframed the site editor. But maybe page/post editor still relies on it.

wp-calypso/apps/wpcom-block-editor/src/wpcom/features/site-editor-env-consistency.js

This one seems for dev/test env to keep the consistency of the origin. I think Jetpack doesn't know that so we might still need to override the Back URL on the Live Preview 🤔 But the goal of this is to replace the origin instead of the destination.

@okmttdhr
Copy link
Member Author

okmttdhr commented Sep 6, 2023

wp-calypso/apps/wpcom-block-editor/src/calypso/features/iframe-bridge-server.js
I guess we might not need this for the site editor since we have un-iframed the site editor. But maybe page/post editor still relies on it.

wp-calypso/apps/wpcom-block-editor/src/wpcom/features/site-editor-env-consistency.js
This one seems for dev/test env to keep the consistency of the origin. I think Jetpack doesn't know that so we might still need to override the Back URL on the Live Preview 🤔 But the goal of this is to replace the origin instead of the destination.

Thanks for your explanation, @arthur791004 👍

Let's move this PR (and site-editor-env-consistency as well, in my opinion) into Jetpack!

@okmttdhr okmttdhr closed this Sep 6, 2023
@okmttdhr
Copy link
Member Author

okmttdhr commented Sep 7, 2023

I've thought about this again, and I don't think the backlink for Live Preview should be changed in Jetpack (or in jetpack/modules/masterbar/admin-menu /class-admin-menu.php at least), because, looking at this comment, the backlink in the Site Editor does not belong to the admin menu.

And I use __experimentalThemePreviewBackLink instead of __experimentalDashboardLink as described in WordPress/gutenberg#54174, so the changes only affect the Site Editor, not the admin menu.

I had thought about implementing it in jetpack-mu-wpcom, but in that case, it would be better to migrate site-editor-env-consistency.js along with it for coherence.

Any thoughts, @arthur791004?

@okmttdhr okmttdhr reopened this Sep 7, 2023
@arthur791004
Copy link
Contributor

because, looking at this comment, the backlink in the Site Editor does not belong to the admin menu.

I think the important part is it belongs here because it's part of changing wp-admin links to their wp.com equivalents. The Site Editor is part of the wp-admin since the URL is /wp-admin/site-editor.php, and that settings are only used by the Site Editor as well ($settings, block_editor_settings_all)

it would be better to migrate site-editor-env-consistency.js along with it for coherence.

I think this is only for the consistency of the origin instead of the link. The better way might be to migrate them to jetpack-mu-wpcom as you mentioned 🙂

We have to avoid adding more and more features to wpcom-block-editor or ETK plugin since we have the plan to migrate them to Jetpack. See p9oQ9f-Ht-p2 and p58i-dox-p2

@okmttdhr
Copy link
Member Author

okmttdhr commented Sep 7, 2023

Thanks for your comment again, @arthur791004, and that all makes sense to me.

@okmttdhr okmttdhr closed this Sep 7, 2023
@okmttdhr okmttdhr changed the title Override the Back on the Live Preview Override the backlink on the Live Preview Sep 7, 2023
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.

3 participants