-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Blocked] Starter page templates: Remove iframe from preview #49101
[Blocked] Starter page templates: Remove iframe from preview #49101
Conversation
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. |
Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com D55737-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2 |
Thanks for testing @glendaviesnz! I'll take a closer look. |
c6be1ba
to
98edc82
Compare
It turns out this issue is caused by the Map block continuing to fire the resize event after the component has been unmounted. To reliably see the TypeError in the console, first select a pattern that includes the Map block (e.g. the second last layout under the Contact category), then select another pattern, and then resize the window. I believe we weren't seeing this in the iframe version because the JS was never running in the iframe context. I've put together a one-line fix in Automattic/jetpack#18489, but I think it should be fairly harmless to ignore the error in the meantime since it doesn't appear to block anything. |
Okay, I've fixed the following:
This is ready for any test/review. I've tested in Chrome, Firefox, and Edge on Mac, however I'm still having issues getting Safari to pick up most hosts file so if anyone gets a chance to test that out, that'd be a huge help! I'll try upgrading to Big Sur and see if that resolves anything with my dev environment. |
Adding the iframe fixed a whole lot of other fundamental issues originally (@retrofox worked on it and might have some links?), so I don't think this is feasible solution. Furthermore, Ganon team is starting a project for redesigned page layout picker (pbAok1-1QJ-p2), so putting too much effort in this now isn't good, unless there are high priority bugs to be fixed? (I didn't look at the list yet) |
The bigger challenge for us was how to deal with the CSS media query. Have you tested the preview in small sizes? I agree that some themes require JS and it's an issue, though. That was something that was on our radar too. |
Ah yes, media-queries of course... I believe there were also CSS leakage and specificity issues cropping up frequently, but it's been a while so might remember wrong. The site-editor itself is now in an iframe, and hopefully soon post-content too (WordPress/gutenberg#21102). Gutenberg is slowly making its way into Calypso as well, and doing so needs to solve some of those asset loading problems similar to here. So I'd expect if we need/want to do this in future, it'll become almost "for free". But for now we've designed to live without the big preview, going forward. |
Thanks for the feedback @simison and @retrofox! The existing issues that this PR is designed to resolve are #48889 (Contact form not rendered correctly), #48883 (Issue with rendering map block in preview) and #48881 (Coblocks Masonry gallery does not render at all). The latter two visual issues are pretty bad, but I wouldn't consider this PR fix high priority, because in the short-term we could always unpublish the problematic layouts. I think a fair bit has changed with how editor styles works since the initial iframe implementation, and in this PR the approach is pretty similar to the existing block previews in the pattern inserter, so at least in manual testing, it's been working quite well, and the existing All that said, given Ganon's work to redesign the layout picker (as you mention with pbAok1-1QJ-p2), I very much agree that it'd be better to avoid a potentially risky refactor if we're going to be throwing out the work in the short-term, anyway. I'm happy to park this PR in lieu of the work to do the layout picker redesign, but it's always here in case the redesign is delayed and we need to add back in layouts that mightn't play nicely with the existing large preview 🙂 I'll update #48883 and #48881 with the suggestion that we unpublish the two or three layouts that are causing issues with the preview, until the redesign. |
I'll share some relevant context in case it's helpful:
|
Update: I've explored a couple of different alternatives to this PR, but I believe this one is still the best candidate for resolving the reported issues (#48881, #48883, #48889). With the way that the iframe is currently implemented, I haven't managed to get the Map block to work at all within the iframe because the block expects direct access to the DOM, and a Returning this PR, it's still working pretty well for me, and I think is the best of the options I've explored. I haven't encountered any CSS bleed issues that I'm aware of, and the scaling of the preview appears to look okay. It doesn't hit the mobile breakpoints, so it means the effective preview size is as though you're previewing how the layout will look on desktop. At this stage, having sunk a bit of time into a few different options, I think our best bet if we'd like to fix the linked issues before the new page layout picker is built, is to test and review this PR and merge it as an interim measure. If the PR isn't suitable, then let's put the linked issues into the backlog until the new page layout picker is built. @simison what do you think? Is it worth us testing this PR out / getting it reviewed and getting these issues fixed in the short-term, or would you prefer to hold off? |
We've started work on the new page picker today. Can't be sure of the "when", but I'd say that if this fixes things for the next month or two until we remove the iframe completely in the redesign, then I think it's safe (if everyone else does). |
@andrewserong meanwhile, do those blocks work in previews rendered from the API in screenshots? |
@simison it looks like the blocks work correctly in the Let us know which way you'd like to go with this PR, happy to proceed to reviewing / getting it in, or park it until the new page layout picker is ready 🙂 |
@andrewserong yah, would be good to have an issue for it at least. There's prolly something that can be adjusted here: https://github.com/Automattic/mShots/blob/748a64f945265cce1786393e43c4e9b43236d2b9/lib/snapshot.js#L174 |
Thanks @simison, I've written up the two issues I've encountered:
I've added the latter to the View backlog. Would @Automattic/ganon have scope to look into the linked mShots issue as part of the page layout picker work? CC: @ramonjd I think with these outstanding issues, and that they'd affect the screenshots on the left hand side in the existing modal, that's enough to tip me over into thinking we should park this PR and resolve the above as part of the new page layout picker work. I'm happy to have a go at the Let me know if there are any objections, otherwise tomorrow I will close this and the alternative PR #49511, and update the linked issues that they're blocked by Automattic/mShots#30 and 216-gh-Automattic/view-design and will be resolved in the new page layout picker work. |
Are there any other examples of pages not rendering correctly in mshots? Or are we sure it's a Javascript execution/load time problem? See comment here Maybe @deBhal has the time to dive into it. If we can't resolve it quickly, or it's limited to mapbox/canvas, perhaps we can think of alternatives, e.g., take a screenshots of a copy of that page, which contains a placeholder image or something? |
Thanks for looking into it @ramonjd.
No, it's definitely more of a hunch than anything I'm sure of! Actually, now that I test it further, I think it might be that one particular block that isn't working correctly in mShots, so the mShots issue might be lower priority, then. I think the remaining issues might then be getting things to work properly in the Update: the two main |
b8ffa16
to
fc61a8a
Compare
@simison while testing other block rendering issues, I encountered a more pressing issue with the existing iframe (Google fonts don't appear to be loading — #49742). I've double-checked and confirmed that this PR would resolve that issue, so I've given this PR another rebase to make sure it's still working correctly. So, there could still be a good case to be made for getting this PR in. What do you think? |
Since there is a different refactor of the starter page templates going on in #49661, I'll close this PR. @Automattic/ganon just a heads-up, if you wind up running into any issues with the large preview in your refactoring / rebuilding work for the page layout picker, feel free to borrow from this PR if it helps 🙂 |
A number of reported issues with the preview in the page layout picker can be resolved by removing the iframe from the preview. Certain blocks (such as masonry, or maps blocks) require JavaScript to complete their layouts in the case of masonry, or to perform API calls in the case of a map block. By removing the iframe, the preview has access to the JavaScript already loaded on the page, so we don't need to add any additional hacks to get these blocks working, and it also inches the preview a bit closer to how the preview works in the pattern inserter.
Issues resolved by this change:
Changes proposed in this Pull Request
Screenshots
Note in the following that the map works in the preview, and that the masonry layout renders:
Testing instructions
CC: @Automattic/ganon — just copying you in, in case you get a chance to review since you're also doing work on this area 🙂