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

Fix editor dynamic width on WP 5.6 #1520

Closed
wants to merge 2 commits into from

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Oct 27, 2020

Fixes #1512.

First, I would like to summarize what this feature is about:

  • Change the editor width depending on the selected template. If the template is full width, the editor area is wider, mimicking the template in the frontend.
  • That needs to happen when the user changes the template in the sidebar + on page load.
  • If the template is not full width, blocks shouldn't allow setting a 'wide' or 'full' alignment.
  • According to the code, I think it was planned it would remove the alignment from all 'wide' and 'full' blocks when the template is changed from full width to default/homepage. However, this feature didn't seem to work and I completely removed the code. With this PR, a 'wide' or 'full' aligned block stays that way even in narrow templates, but in those cases they render like a centered block.

While this PR fixes the issue and (I think) cleans up a bit the code, I still fill this feature is very fragile. We are directly interacting with the DOM of the editor, so the code might break every time there is an update in the editor markup in a future WP update. In fact, that same code stopped working some months ago with WP 5.4 (see #1316).

I'm not sure if there is a better way to handle this with proper APIs, if there is, I couldn't find it but I would be happy to be corrected, so we could fix this issue in a robust way. In case there isn't any supported API, we have several options:

  • Fix it (with this PR or another one) and keep an eye on this feature for future breakages, keep fixing it every time a WP update breaks this code.
  • Fix it for now, but remove this feature next time it breaks.
  • Take the chance now and instead of fixing it, remove the feature.

Thoughts @haszari @nerrad?

In addition to that, I opened an issue in Gutenberg repo with our use-case. You can find it here: WordPress/gutenberg#26494. We would also need WordPress/gutenberg#18571 to be fixed in order to be able to completely re-write this feature using supported APIs.

Screenshots

Peek 2020-10-27 09-21

How to test the changes in this Pull Request:

  1. Create a page and switch to the 'Full width' template.
  2. Add a paragraph block that spans several lines + a cover block with alignment wide + a cover block width alignment full.
  3. Save the page, preview it in the frontend and reload it in the editor. Verify styles are applied correctly in the frontend + editor.
  4. Now, switch to the 'Default' template and verify the paragraph block is narrower and the cover blocks look like if they were center-aligned. Click on them and verify the 'wide' and 'full' alignment options can't be selected.
  5. Save the page, preview it in the frontend and reload it in the editor. Verify styles are applied correctly in the frontend + editor.

Do the testing steps above with WP 5.5 and WP 5.6.

I also tested it with IE11 to ensure code is compatible with it.

Affected flows & features:

  • Merchant can author pages that use the full page width.
  • Block editor shows a realistic preview of the front end styling.

Changelog

Compatibility – The code to adapt the editor width to the selected template has been updated so it works with WP 5.6.

@nerrad
Copy link
Contributor

nerrad commented Oct 27, 2020

While this PR fixes the issue and (I think) cleans up a bit the code, I still fill this feature is very fragile. We are directly interacting with the DOM of the editor, so the code might break every time there is an update in the editor markup in a future WP update. In fact, that same code stopped working some months ago with WP 5.4 (see #1316).

I agree with your concerns here and I also have some concern around potential impacts to performance with the mutation observer.

I also wonder whether Storefront should have even been doing this in the first place. I realize that it was intended to show the impact of the default template (when it has a sidebar) on the resulting container widths in the Editor, but I think that is potentially misleading because there also is no sidebar displayed in the editor.

If anything, I think there are some inconsistencies with certain blocks styling that changed in WP 5.6 that might need addressed specifically to match behaviour in storefront frontend, but I don't think we should worry about changing the actual width of the editor container should change dynamically according to the template selected because it's still not really accurate right? I'm also wary of establishing an expectation here around template selection when this could potentially change as FSE rolls out more broadly.

I'd prefer to drop this feature and see what kind of pushback we get. Since it's editor only facing the impact is limited to store admins.

@haszari
Copy link
Member

haszari commented Oct 29, 2020

I'd prefer to drop this feature and see what kind of pushback we get. Since it's editor only facing the impact is limited to store admins.

I completely agree – let's take the opportunity to make Storefront simpler and more robust.

@Aljullu
Copy link
Contributor Author

Aljullu commented Nov 2, 2020

Thanks for the feedback @nerrad and @haszari. I opened another PR to completely remove this feature: #1525.

@Aljullu Aljullu closed this Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Issues related to the editor styles status: needs review PR that needs review type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WordPress 5.6 compatibility – editor width doesn't change based on full-width/default template
3 participants