Use iframes in legacy widget preview. #14643
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
requested review from
aduth,
ajitbohra,
gziolo,
noisysocks,
notnownikki,
Soean,
talldan and
youknowriad
as code owners
jorgefilipecosta
added
[Type] Enhancement
|
Thanks for working on this Jorge, I think this should be the way to go to preview widgets. I'm having some errors though
Also the "edit" part of the widget is not loading. (I'm using the site origin widgets) |
jorgefilipecosta
added
the
Needs Technical Feedback
|
What's the status of this one? Looks like it needs a rebase 🙂 |
jorgefilipecosta
added
the
[Status] Blocked
|
Hi @noisysocks yes we need to rebase this PR. I don't think we should rebase it before #15801 is landed, otherwise we will waste effort. PR #15801 has higher priority. I marked this PR as blocked. |
|
Should we refresh this or close it and make an issue out of it? This is still something we need and a good idea! @adamziel 's idea was that depending on the min browser support we have we could use shadow DOM instead iframes. |
adamziel
force-pushed
the
update/use-iframes-for-widget-preview
branch
from
1fbde54 to
53651fa
Compare
|
I rebased and refreshed this PR. It's still not quite there yet, but it's close:
|
|
Size Change: +248 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
adamziel
reviewed
Sep 18, 2020
| const HEIGHT_MARGIN = 20; | ||
| const [ height, setHeight ] = useState( DEFAULT_HEIGHT ); | ||
| const currentUrl = document.location.href; | ||
| const siteUrl = currentUrl.substr( 0, currentUrl.indexOf( 'wp-admin/' ) ); |
There was a problem hiding this comment.
Anyone aware of any better way of getting site's URL?
There was a problem hiding this comment.
It is available in the REST API index at wp-json.
There was a problem hiding this comment.
Actually there is another place in navigation editor that could also use this correction - let's address both in a separate PR
|
This PR is ready for review now cc @kevin940726 @draganescu @jorgefilipecosta |
draganescu
reviewed
Sep 21, 2020
draganescu
approved these changes
Sep 21, 2020
ellatrix
reviewed
Nov 5, 2020
| } ); | ||
| return ( | ||
| <Disabled> | ||
| <FocusableIframe |
There was a problem hiding this comment.
Why is FocusableIframe used if the onFocus prop is not?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates legacy widget block to use an iframe for the preview.
Using an iframe allows the preview to be more reliable as we can load frontend styles and scripts there.
How has this been tested?
I used the ultimate social media icons for testing purposes.
After
Before