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

Live Preview: Fix the Save button wasn't overriden correctly #95639

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

arthur791004
Copy link
Contributor

Related to #95611

Proposed Changes

  • Fix the error when trying to save/activate changes during the preview.
  • The reason is wpcom would redirect to wordpress.com/checkout/:site/:plan page if the site didn't have the ability to activate the previewing theme. However, there was CORS issue so the error would show.
  • We have a mechanism to override the Save button to open the new tab. However, it didn't work because the button was still not visible when we tried to override it. As a result, this PR made changes to override the button only when the editor is ready.
Before After
image image

Why are these changes being made?

Testing Instructions

  • Apply changes to your site
  • Sandbox widgets.wp.com and remember to disable the cache
  • Go to Theme Showcase on your free site
  • Preview any non-free theme
  • Switch to the edit mode
  • Make sure you can see the "Upgrade now" button
  • Refresh the page
  • Make sure you can still see the "Upgrade now" button (there is a little delay...)

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests 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-aUh-p2)?

@arthur791004 arthur791004 self-assigned this Oct 24, 2024
@matticbot
Copy link
Contributor

matticbot commented Oct 24, 2024

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/live-preview-activate on your sandbox.

@arthur791004 arthur791004 requested a review from a team October 24, 2024 03:40
@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 Oct 24, 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.

}
// Delay it to ensure the element is visible.
const timeout = window.setTimeout( () => {
if ( canvasMode === 'view' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe we don't need the delay in view mode? I don't think it's broken. The delay itself makes it a bit confusing 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's more consistent for both view and edit mode and it's safer for any dom manipulation regardless of the order of loading.

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

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

In refreshing edit mode, I am seeing the "You are previewing..." banner first and then it gets replaced with "Get access to..." banner. I don't see this behavior in production.

Screen.Capture.on.2024-10-24.at.11-35-35.mp4

@fushar
Copy link
Contributor

fushar commented Oct 24, 2024

In refreshing edit mode, I am seeing the "You are previewing..." banner first and then it gets replaced with "Get access to..." banner. I don't see this behavior in production.

Maybe we can live with this tradeoff because we assume that user will first open the view mode first while the theme is still loading 🥲

@arthur791004
Copy link
Contributor Author

In refreshing edit mode, I am seeing the "You are previewing..." banner first and then it gets replaced with "Get access to..." banner. I don't see this behavior in production.

I made changes to check whether the editor is ready only for the upgrade button and it seems to look better.

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

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

It's much better now 👍

Wondering why it broke? I am pretty sure it worked fine in the past 🤔

@arthur791004
Copy link
Contributor Author

Wondering why it broke? I am pretty sure it worked fine in the past 🤔

I'm not sure but maybe something changed in Gutenberg 🙈

@arthur791004 arthur791004 merged commit 5fdb927 into trunk Oct 24, 2024
11 checks passed
@arthur791004 arthur791004 deleted the fix/live-preview-activate branch October 24, 2024 07:53
@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 Oct 24, 2024
fushar pushed a commit that referenced this pull request Oct 24, 2024
* Live Preview: Fix the Save button wasn't overriden correctly

* Check whether the editor is ready only for the upgrade button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants