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

Legacy widget's preview functionality is broken when the page is moved #34184

Closed
wants to merge 32 commits into from
Closed

Legacy widget's preview functionality is broken when the page is moved #34184

wants to merge 32 commits into from

Conversation

anton-vlasenko
Copy link
Contributor

@anton-vlasenko anton-vlasenko commented Aug 19, 2021

Description

Legacy Widget block has a preview function. However, it uses a relative URL.
iframe cannot load the widgets.php file when the code is no longer running from the /wp-admin/ URL.
This PR fixes that by using a REST API endpoint instead of the widgets.php file to fetch preview HTML. It doesn't depend on the admin URL.
Fixes #30049

Based on #34230

How has this been tested?

  1. Go to Appearance -> Widgets page.
  2. Click on the "plus sign" button at the very bottom of the page.
  3. Add a Legacy Widget block.
  4. Select Meta widget from the dropdown.
  5. Click somewhere on the background of the page (not on the Meta widget itself). You should see a preview of the Meta widget now.
  6. Open your browser's developer tools and find an <iframe /> element with the wp-block-legacy-widget__edit-preview-iframe class. Check its srcdoc attribute.

Expected result:

The srcdoc attribute of the <iframe /> should contain html code of the preview.
Please see the screenshots section for reference.

Screenshots

Screenshot 2021-08-12 at 20 00 39
Screenshot 2021-08-19 at 21 59 05

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@anton-vlasenko
Copy link
Contributor Author

anton-vlasenko commented Aug 25, 2021

I've merged #34230 into this PR to make the e2e tests pass.
So please ignore the code from #34230 during the code review.
UPD: now it should only show changes related to this PR because I've changed the target branch.

@kevin940726
Copy link
Member

@anton-vlasenko You can target the base branch to #34230's branch so that it won't show the commits from that PR. Just have to remember to rebase to trunk once it's merged :).

@anton-vlasenko
Copy link
Contributor Author

@kevin940726

@anton-vlasenko You can target the base branch to #34230's branch so that it won't show the commits from that PR. Just have to remember to rebase to trunk once it's merged :).

That's a good idea. I tried to do it as you said and I got this popup message:
Screenshot 2021-08-25 at 11 54 32
I understand it's doable but I'm afraid to screw something up.

@anton-vlasenko anton-vlasenko changed the base branch from trunk to add/legacy-widget-rendering-api August 25, 2021 10:08
@anton-vlasenko anton-vlasenko changed the base branch from add/legacy-widget-rendering-api to trunk August 25, 2021 10:10
@kevin940726
Copy link
Member

@anton-vlasenko I believe you just have to remove the commits that's already on that branch and rebase your branch onto that.

@anton-vlasenko anton-vlasenko changed the base branch from trunk to add/legacy-widget-rendering-api August 25, 2021 13:50
adamziel and others added 23 commits August 27, 2021 14:12
…iew was loaded.

2. Refactor the function. Use return early approach.
2. Add error handling.
3. Abort request if the iframe is unmounted.
4. Use useEffect to make a request (per kevin940726's comment).
5. Use states instead of manipulating DOM values.
…dability.

2. Remove isPreviewFetched state (useEffect is enough).
3. Add idBase and instance as dependencies to useEffect method. We have to fetch the preview html again if any of those parameter gets changed.
4. Move abortController and fetchPreviewHTML inside the useEffect callback to avoid recreating them on each component redraw.
We have to include id_base parameter.
Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍. We need to wait for #34230 before merging this though, or just merge this onto that branch.

@adamziel adamziel force-pushed the add/legacy-widget-rendering-api branch from 09a1d67 to c6b6664 Compare August 30, 2021 10:30
@adamziel adamziel deleted the branch WordPress:add/legacy-widget-rendering-api August 30, 2021 11:17
@adamziel adamziel closed this Aug 30, 2021
@anton-vlasenko
Copy link
Contributor Author

This PR has been closed by GitHub (unintentionally).
So let's continue with the review here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Widgets preview uses a relative URL that breaks when the page is moved
4 participants