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

Resizable editor experiment #19082

Open
wants to merge 19 commits into
base: master
from
Open

Resizable editor experiment #19082

wants to merge 19 commits into from

Conversation

@tellthemachines
Copy link
Contributor

tellthemachines commented Dec 12, 2019

Description

This PR addresses the wish for a resizable editing canvas expressed in #13203.

** UPDATE **

This is ready for proper reviewing now!

Here's what it does:

When a breakpoint button is clicked, it

  • resizes the canvas,
  • grabs the relevant stylesheets and removes all media queries that don't apply to that breakpoint, storing them in an object for future use,
  • goes through the storage object and adds back any previously removed media queries that now apply.

How has this been tested?

Tested on all major browsers including IE. Unit and e2e tests have been updated.

Screenshots

Screen Shot 2020-01-21 at 6 27 30 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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. .
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 12, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 13, 2019

Thank you for working on this.

Overall it seems like a promising path forward. A responsive editing canvas can show an instant effect of any responsive-specific changes you mean to make to each block. As a proof of concept, this PR is a great way to surface the challenges we must tackle, UI-wise:

  • how can we make the block settings contextual to breakpoints?
  • how can we make it clear what breakpoint block settings apply to?
  • do we handle arbitrary breakpoints, or just a few?

These questions would be great for us to think about as we have to overcome them in order to truly fix #13203.

GIF:

test

Techwise this drops the iframe and adds something else. It would be good to sync up with Jorge as suggested above, as he's thought a lot about this; can the efforts be combined? There are some toolbar issues, the responsive behavior of the Media & Text block changes after toggling one of the size buttons, and there isn't room for the buttons on smaller screens (where they might not be necessary?) — and also isn't top toolbar compatible.

The UI problems we need to solve together, so I'm adding a label to add some reinforcements. A dropdown is one idea, are there others?

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Dec 17, 2019

@jasmussen you've posed some very good questions, and I will answer with some more questions 😄

how can we make the block settings contextual to breakpoints?

I assume you're thinking about this kind of situation?
Technically we can do that by storing the breakpoint state so we can share it across components. We'll need to do that anyway to render the size buttons properly. (Currently they're looking so broken because I just dumped them into EditorRegions and positioned them over the toolbar 😅 )
The bigger question is probably

how can we make it clear what breakpoint block settings apply to?

I think we could change settings over silently if we have a specific setting for each breakpoint, such as the paddings and margins in @getdave 's PR. But what happens to controls like the Media & Text's Stack on mobile? Does it become a Stack button that is only available on the mobile breakpoint?

do we handle arbitrary breakpoints, or just a few?

If we handle arbitrary ones, we'll need some sort of "resize" handle to drag the canvas borders around. What would that look like? And would it be instead of, or in addition to, the current size buttons? How much value will this bring over having just the buttons, given that most themes only define 2-3 breakpoints anyway?
Also, assuming contextual settings, we'll still have to define discreet breakpoints to switch them over at.

the responsive behavior of the Media & Text block changes after toggling one of the size buttons

Ugh, that's because it has a max-width query set to override the 2 column default, and my crappy code isn't dealing with that scenario very well 😛 . I'll have a look at @jorgefilipecosta 's implementation and see how we can consolidate.

Random thought: if we could use CSS variables, we'd be able to lose half these pesky media queries 😂 😭

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 17, 2019

Awesome thoughts, thank you. I'm working through this in my head as we discuss, so it's very helpful.

The bigger question is probably [how can we make it clear what breakpoint block settings apply to?]

I think we could change settings over silently if we have a specific setting for each breakpoint, such as the paddings and margins in @getdave 's PR.

Yes exactly. It's definitely a UI challenge. There should be some indication in context of a block setting whether it applies to all breakpoints, or just one, probably defaulting to all.

If we handle arbitrary ones, we'll need some sort of "resize" handle to drag the canvas borders around.

It would definitely be the easiest if we specified breakpoints for where change happened to how blocks behave, sort of like how Bootstrap (not a fan) has specific preset breakpoints. It would make for a simpler and more predictable UI, including for the reasons you mention.

The question here, and there's probably an FSE discussion here too, what role does the theme play, if any? Do the responsive breakpoints need to sync up with breakpoints defined by the theme? Perhaps the simplest path forward is to define just phone, tablet and desktop breakpoints for now, and attach block settings to those three. Then potentially we could look at letting themes customize those three, not necessarily add additional ones. CC: @epiqueras and @jffng regarding the FSE aspect.

Random thought: if we could use CSS variables, we'd be able to lose half these pesky media queries 😂 😭

I think there's an aspect of progressive enhancement here that we could explore. I personally feel that it would be acceptable to not show this UI to IE11 at all, thereby enabling us to use CSS variables. CC: @youknowriad in case you have thoughts. But the cost of making this work with IE11 should not be so high it makes us write worse software.


So, this is a big effort, but it's important we cut it up into small pieces on a road-map. Let's zoom out and discuss a hypothetical. The goal (not just of this single PR but of the effort) is:

  • Let the user preview in the editor responsive breakpoints.
  • Let users customize block settings, when appropriate, for each breakpoint.

Let's see if we can reduce scope:

  • We use CSS variables and don't show this UI to IE11.
  • We limit the effort to three breakpoints: phone, tablet, desktop.

Things that can be explored in the future:

  • Letting themes customize the dimensions of each breakpoint.

The above hypothetical suggests the following tasks:

  1. Figure out a UI for showing these breakpoints that scales to mobile. If we go with the dropdown, this one, we need to take into account that a dropdown is also currently being explored for switching between template parts in full site editing (this one, from #19141). Perhaps it's fine these are separate as one is a separate site editing screen, and the other is a content editing screen.
  2. Address the technical challenges involved with a resizable responsive editing canvas sans iframes.
  3. Figure out a UI for a) showing which breakpoint you're customizing and b) whether a value is customized on various breakpoints. Like, having a dropcap on desktop and tablet breakpoints, but not on mobile.

3 is perhaps/probably a separate PR, but it is something to think about.

@tellthemachines tellthemachines force-pushed the try/resizable-editor branch from 709e363 to 86b78bd Dec 18, 2019
@tellthemachines tellthemachines requested a review from ellatrix as a code owner Dec 18, 2019
@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Dec 18, 2019

So, this is a big effort, but it's important we cut it up into small pieces on a road-map. Let's zoom out and discuss a hypothetical.

Thanks for this @jasmussen !

I have integrated @jorgefilipecosta 's work into this PR; the Media & Text block is now rendering correctly for all breakpoints.

However, there are still issues wherever inline styles are used: in the post title, or on column blocks with custom widths. It is in this kind of situation that I think CSS variables could be really helpful, but using them means that anyone on IE won't be able to preview their custom column widths. Does this fit our definition of progressive enhancement?

I'm more than happy to have a play with some of the blocks (on separate PRs, for clarity) and see how much code we can remove/optimise by using a mix of CSS variables and grid 😁

Regarding the scope of the current PR, I think it makes sense to limit it to getting a simulation of 3 breakpoints working correctly, so I guess that leaves the question of where best to put the Desktop/Tablet/Mobile buttons. (I haven't touched them yet so they're still looking as broken as before.)
Would the top toolbar be an appropriate place, shrinking into a dropdown on smaller screens? Or perhaps we should leave out this feature entirely for mobile users for now, and work on @MichaelArestad 's solution(which I really like) as a further iteration? Tempted to do the latter in order to keep this changeset small.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 18, 2019

I have integrated @jorgefilipecosta 's work into this PR; the Media & Text block is now rendering correctly for all breakpoints.

This seems to be working pretty well! Impressive.

However, there are still issues wherever inline styles are used: in the post title, or on column blocks with custom widths. It is in this kind of situation that I think CSS variables could be really helpful, but using them means that anyone on IE won't be able to preview their custom column widths. Does this fit our definition of progressive enhancement?

This is my personal opinion, and it could use a sanity check from folks such as @youknowriad or @mtias. But my thinking is that by keeping the current preview button for IE11, users will have a functional if not as convenient editor, while allowing us to write better and more maintainable software for modern browsers.

Regarding the scope of the current PR, I think it makes sense to limit it to getting a simulation of 3 breakpoints working correctly, so I guess that leaves the question of where best to put the Desktop/Tablet/Mobile buttons. (I haven't touched them yet so they're still looking as broken as before.)

I am working on an updated mockup to accomplish this, but it is in the vein of the previously shown preview dropdown. It feels worth testing, given that it does not break the "Top Toolbar" functionality.

Question here: could we make it so only themes that register an editor-style get this dropdown menu? The point here would be that for a theme that does not, the preview is inaccurate anyway and they might as well get the classic preview button, whereas themes that do opt in, are likely to be closer to the WYSIWYG necessary for the in-editor preview to make sense.

@MichaelArestad 's solution(which I really like) as a further iteration? Tempted to do the latter in order to keep this changeset small.

Michael's solution is clever in that it allows a person on a phone to edit/preview their desktop site. However I would also agree it's something to explore separately, and even subsequently, to the inital preview work. One of our biggest challenges here is to ensure uncomplicated UI, and it would be good to ensure the baseline featureset is in a good place first. This will also likely inform the final look and feel of the phone/desktop preview feature (which I agree that would be cool, btw.)

Bringing responsiveness to the editing canvas, including phone and tablet, is a mammoth achievement unto its own, worth celebrating. 🎉

@MichaelArestad

This comment has been minimized.

Copy link

MichaelArestad commented Dec 18, 2019

This is my personal opinion, and it could use a sanity check from folks such as @youknowriad or @mtias. But my thinking is that by keeping the current preview button for IE11, users will have a functional if not as convenient editor, while allowing us to write better and more maintainable software for modern browsers.

As a proof of concept, I think we can rely on CSS variables. That said, it feels like a good solution from a technical perspective and not a theming or user perspective. It is a concept the persone creating a theme would have to know about ahead of time when building a Gutenberg theme rather than something that just works as expected.

I am working on an updated mockup to accomplish this, but it is in the vein of the previously shown preview dropdown. It feels worth testing, given that it does not break the "Top Toolbar" functionality.

Yesss! Happy to help with that. I also wonder if the height of the preview should more reflect a phone/tablet size.

could we make it so only themes that register an editor-style get this dropdown menu?

We could for now. In the future, I hope that 'editor-style' will be deprecated in favor of a single canonical stylesheet (or a single set of stylesheets). This is probably a good topic for discussion in another issue.

Michael's solution is clever in that it allows a person on a phone to edit/preview their desktop site. However I would also agree it's something to explore separately, and even subsequently, to the inital preview work.

+1 I wouldn't attempt what is in my mockup until we have this first step of a responsive preview complete.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 18, 2019

However, there are still issues wherever inline styles are used: in the post title, or on column blocks with custom widths. It is in this kind of situation that I think CSS variables could be really helpful, but using them means that anyone on IE won't be able to preview their custom column widths. Does this fit our definition of progressive enhancement?

and

As a proof of concept, I think we can rely on CSS variables. That said, it feels like a good solution from a technical perspective and not a theming or user perspective. It is a concept the persone creating a theme would have to know about ahead of time when building a Gutenberg theme rather than something that just works as expected.

Help me understand the nuance in case I'm missing something.

To my understanding the usage of CSS variables will be purely a "behind the scenes" thing to help orchestrate the responsiveness of the editor style, and not something any WordPress theme developer should ever need to worry about, is that correct?

@MichaelArestad

This comment has been minimized.

Copy link

MichaelArestad commented Dec 18, 2019

To my understanding the usage of CSS variables will be purely a "behind the scenes" thing to help orchestrate the responsiveness of the editor style, and not something any WordPress theme developer should ever need to worry about, is that correct?

Ah! If that's the case, I misunderstood. Carry on!

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 18, 2019

It's good to clarify!

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Dec 18, 2019

Question here: could we make it so only themes that register an editor-style get this dropdown menu? The point here would be that for a theme that does not, the preview is inaccurate anyway and they might as well get the classic preview button, whereas themes that do opt in, are likely to be closer to the WYSIWYG necessary for the in-editor preview to make sense.

Based on @ellatrix 's comment in my previous PR I gather that theme editor-styles will become unnecessary in the (near?) future, so I'm not sure it would make sense to do that at this point.
The other thing is that even if the editor view doesn't fully reflect the output, the resizing controls would still be needed to enable breakpoint-specific settings.

I wonder if we should touch the preview button at this point, or if it would be better to have the resizing controls somewhere else (or even next to the preview button?), to make it clear that they apply to the editing canvas. Users know that preview is a separate page, so adding same-page-related controls to it might be confusing?

To my understanding the usage of CSS variables will be purely a "behind the scenes" thing to help orchestrate the responsiveness of the editor style, and not something any WordPress theme developer should ever need to worry about, is that correct?

That was my idea! Using variables will help us avoid using inline styles. That will improve the canvas resizing experience and it has the added benefit of making it easier for themes to override these styles 🙂

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Dec 19, 2019

Update: for this PR, two major issues remain:

  • The UI for the resize controls (awaiting design)
  • How to transform only the relevant styles.

For the second issue, we need to bear in mind that while the Gutenberg plugin has a separate stylesheet for each package, in Core they are all bundled together with wp-admin styles.
We'll need to do some work in Core to separate out the styles to transform into another stylesheet. One possibility would be to load them inline in the footer but that might cause a FOUC.
Otherwise we'll need to introduce a custom step in the build to either create a separate stylesheet, or inline the styles we need in the head. I'm not hugely familiar with this aspect of Core, so would be awesome to have some opinions on this 😄

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 19, 2019

I'm working hard to get you designs and hope to have something we can work with before my break next week!

How to transform only the relevant styles.

Can you elaborate on this?

Are you referring to editor styles? Or editor styles made responsive by the system?

I have a vague idea the answer, for now, might be to inline them in <style> tags.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 19, 2019

@tellthemachines Alright I have some mockups to share. The visual style is inspired by the UI explored in #18667, but the principle should work for the UI we have today:

V1, Preview Dropdown

By combining the controls into a single dropdown, we are implying that what you are seeing in the editor is a preview, and grouping with that the options to emulate Tablet and Phone dimensions:

V1, Phone Breakpoint

To me, that's the V1:

  • The impressive responsive preview
  • The dropdown combining it all
  • Preview externally remains an option

I have some ideas for how we can integrate saving properties for each breakpoint as well, and I hope to share that separately.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Dec 19, 2019

Thanks for the mockups @jasmussen ! I'll start building that dropdown 😄

How to transform only the relevant styles.

Can you elaborate on this?

To make the editing canvas "responsive" we're grabbing all the stylesheets that are relevant to its content, and enabling/disabling media queries depending on the selected breakpoint. (This is why it doesn't work with inline styles.)
The problem is, while in the plugin the stylesheets for each package are separate, once it goes into core they're all bundled into a single stylesheet, so currently it's impossible to isolate the styles that we need to manipulate.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 20, 2019

I'll start building that dropdown 😄

Thank you! I think it will be helpful to test out in actual practice to see how such a popover feels.

Is there a better term for "Preview externally"? As noted, this menu item would absorb the functionality of the existing Preview button, which it to say it opens the theme preview in a new tab. Would "Preview in new window" or "Preview in new tab" be a better label? CC @pablohoneyhoney in case you have thoughts.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 20, 2019

To make the editing canvas "responsive" we're grabbing all the stylesheets that are relevant to its content, and enabling/disabling media queries depending on the selected breakpoint. (This is why it doesn't work with inline styles.)
The problem is, while in the plugin the stylesheets for each package are separate, once it goes into core they're all bundled into a single stylesheet, so currently it's impossible to isolate the styles that we need to manipulate.

:keanu-whoah:

I am failing to grasp the full extent, but it does sound like a use case, potentially, for CSS variables.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 17, 2020

@jasmussen , I couldn't reproduce the horizontal scrollbar. I wonder if it was any specific block you were using that caused it? Anyway, I increased the width to 360px so it should be fixed now.

I also added the padding to tablet mode, and made it 36px for screens < 800px.

For the min-height, to make it work I had to add the same min-height to the sidebar, otherwise it ends up looking like this when scrolled:
Screen Shot 2020-01-17 at 3 01 41 pm
A slight drawback is that now, when in mobile or tablet mode, the sidebar will be scrollable to that height even if all its sections are closed. How big of an issue do reckon that is?

Regarding the dropdown as a pattern, I used existing components to build it: Button, Dropdown, MenuGroup and MenuItem, plus a few icons; I wrapped the top three items in a <fieldset> to give the "View" label some semantic context, but that's pretty specific to this use case. The only really "new" part is the styles I attached to these components. Could we perhaps abstract these styles into mixins that can be used with other components when needed?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 17, 2020

A slight drawback is that now, when in mobile or tablet mode, the sidebar will be scrollable to that height even if all its sections are closed. How big of an issue do reckon that is?

I think that's worth fixing. If you don't mind I can probably jump in and take a look. I've a few things on my table today, but I should be able to get to it.

The only really "new" part is the styles I attached to these components. Could we perhaps abstract these styles into mixins that can be used with other components when needed?

I think that's for reviewers to help decide. I'm fine shipping that bit as is!

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 17, 2020

I noticed a quick thing; if you are using a theme that does not style the background color of the editor, you get a gray background:

Screenshot 2020-01-17 at 11 34 22

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 17, 2020

Also, I will throw myself on a sword. You were completely right about this:

For the min-height, to make it work I had to add the same min-height to the sidebar, otherwise it ends up looking like this when scrolled:

Can we try a max-height instead of a min-height? And if you feel that looks terrible too, or has weird side effects, maybe we just drop my silly idea after all.

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Jan 19, 2020

This is amazing! Bravo, bravo 👏

Some notes from my testing:


  1. Comparing it to the More menu, this new View menu looks a little off:

Screen Shot 2020-01-20 at 10 46 00

Screen Shot 2020-01-20 at 10 46 37

  • The horizontal separator is black, not grey.
  • The group label (VIEW) is capitalised, not sentence-case.
  • Both menus use View as a group label. Should we should pick a different title so as to disambiguate?
  • The checkmark in the View menu is bigger than the one in the More menu.
  • It might be nice to add the external icon to the Preview externally button so as to hint to the user that a new tab will open.

  1. It's strange that in Tablet view the inserter appears really narrow:

Screen Shot 2020-01-17 at 16 15 41

Relatedly, some of the inserter icons appear cropped in Tablet mode:

Screen Shot 2020-01-20 at 10 50 32

Also, the inserter appears really narrow when in Mobile mode:

Screen Shot 2020-01-20 at 10 54 47


  1. In the Mobile or Tablet views, hovering over the grey area and scrolling causes the emulated viewport to move:

scrolling

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 20, 2020

Will it be possible for developers to disable/hide this functionality?


return array_merge( $settings, $stylesheets_settings );
}
add_filter( 'block_editor_settings', 'gutenberg_experiments_editor_settings' );

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

The argument passed here ('gutenberg_experiments_editor_settings') doesn't match the name of the function above (gutenberg_stylesheets_editor_settings).

@@ -0,0 +1,26 @@
<?php

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

What's the benefit of this being in its own file as opposed to in e.g. client-assets.php?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jan 21, 2020

Author Contributor

Hmm, good point, no real benefit.

} );
}

export default function useSimulatedMediaQuery( partialPaths, width ) {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

Since it's a "public function", I think let's add a docblock comment here explaining the hook.

} );
return () => {
originalStyles.forEach( ( rulesCollection, styleSheetIndex ) => {
if ( ! rulesCollection ) {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

When would this condition be true? If I'm reading it right, this if is dead code and can be deleted.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jan 21, 2020

Author Contributor

If there are no replaceable media rules, originalStyles will be empty so that condition should run. In the current use case, it's unlikely that will ever happen, but if this hook is used in another context, it might?

},
[ partialPaths, width ]
);
return null;

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

Is this return necessary? If we omit it then I think it makes it clearer that one shouldn't use the return value of this function.

let deviceWidth = 0;

switch ( device ) {
case 'Tablet':

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

Related to comment above about storing translated text in state: these switch statements won't work if the user's language is not set to English.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jan 21, 2020

Author Contributor

😅


window.addEventListener( 'resize', () => updateActualWidth( window.innerWidth ) );

const canvasWidth = ( device ) => {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

It's somewhat personal preference but I think function names should start with a verb e.g. getCanvasWidth or calculateCanvasWidth.

savePost: dispatch( 'core/editor' ).savePost,
toggleCanvas( deviceType ) {
dispatch( toggleCanvasWidth( deviceType ) );
},

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

Any reason to not match the style of the two other actions here?

toggleCanvasWidth: dispatch( 'core/block-editor' ).toggleCanvasWidth,

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jan 21, 2020

Author Contributor

Huh, not sure why I did that 😕

*
* @return {string} Device type.
*/
export function deviceType( state ) {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

I think this should be getDeviceType so that it matches the naming convention of our other selectors.

<Path fill="none" d="M0 0h24v24H0z" />
<Path d="M9 16.2L4.8 12l-1.4 1.4L9 19 21 7l-1.4-1.4L9 16.2z" />
</SVG>
);

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jan 20, 2020

Member

Not related to this PR, but noticed we have inline SVGs all over the codebase now. Might be nice to consolidate them into a single file/package so that we don't accidentally end up with duplicated code in the bundle.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jan 21, 2020

Author Contributor

Yeaaah, @jasmussen mentioned there would be some redesigning of icons, so perhaps that's a task for when we have all the definitive icons sorted out?

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 20, 2020

Also, I will throw myself on a sword.

😂 Please don't!
I updated to use max-height, but increased the heights a bit because I thought they looked a bit squashed, and also, if users have big screens, why not give them more editing space? I also added a default background color to the content area so it doesn't look grey if the theme doesn't add a background.

Regarding @noisysocks' comments on the menu styling as compared to the "More" menu, I'll defer to @jasmussen, whose design I tried to follow as closely as possible. I have added the external preview icon from Dashicons as a temporary measure.

The block inserter menu issue is interesting :lolsob:
I have added some styles to make it look not squashed, but when searching, if only a single letter is added, and heaps of options appear, there is no way to constrain the height of the popover. On desktop it has a height, but of course that's being removed by the media query manipulation 😅
I'm kinda tempted to add a class to .edit-post-editor-regions__content when it's resized to tablet or mobile mode, so we can attach style overrides for these annoying situations. What are everyone's thoughts on this?

@chrisvanpatten currently, the dropdown is only shown on > medium sized screens, and the classic preview button is still there for smaller screens, so it's still possible for a developer to hide the dropdown altogether with CSS and force the classic one to show for all screen sizes. However, we plan on extending this functionality to mobile at some point, so this might change in the future. How big of an issue do you reckon this might be?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 20, 2020

😂 Please don't!

I won't, even if it was a metaphorical one. Though perhaps I should, because if the max-height doesn't work either, perhaps we should just leave that whole bit out for now, and focus only on the width, not the height. After all, it is easy to revisit this, and perhaps we should leave any smarts out here and ponder them separately!

Regarding @noisysocks' comments on the menu styling as compared to the "More" menu, I'll defer to @jasmussen, whose design I tried to follow as closely as possible. I have added the external preview icon from Dashicons as a temporary measure.

Thank you for doing that. I think for this one we should do what Robert suggests and remove as much custom styling as we can. Not because I don't appreciate your efforts, but in order to keep this PR as focused as we can. In #18667 a new UI has been outlined, and in #19344 I've tried implementing that. There will be a separate PR that looks at the UI surrounding the editing canvas also. I suspect it's best to keep the new styles focused to those two branches, so they can roll out in one go!

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 20, 2020

Will it be possible for developers to disable/hide this functionality?

Can you expand a little bit on this?

This reminds me, @tellthemachines did we implement it so that the preview dialog only appears if the theme registers an editor style? I.e. add_theme_support( 'editor-styles' );? And if we added that, would this be sufficient "opt-in", or do you find a use case for further opt-in, Chris?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Jan 20, 2020

Thanks for your feedback @jorgefilipecosta! I've added the array of stylesheets as an editor setting. This works for the plugin, but will I be able to access these editor settings in core?

Hi @tellthemachines, yes in the core we can also pass and change the editor settings. The complex part is diving core styles into two different requests one containing the styles that should be transformed (blocks library, block editor, etc) and another containing styles that should not be transformed e.g: edit-post.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 20, 2020

@jasmussen

Will it be possible for developers to disable/hide this functionality?

Can you expand a little bit on this?

Headless/API-driven installs (like ours), which don't have a direct relationship between editor presentation and front-end presentation, are becoming increasingly common. A tool like this — while undoubtedly useful for a number of folks — will introduce more confusion for our users.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 20, 2020

@chrisvanpatten Thank you for sharing the use-case, I see it.

If we made it so the dropdown only appears if the theme registers an editor style (add_theme_support( 'editor-styles' );) would that work for you? You'd essentially have to opt-in that way.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 20, 2020

@jasmussen Yeah, I think editor-styles support as a proxy for this type of visual feature makes a lot of sense. It indicates you've made the effort to create a styled editor experience, and would benefit from related enhancements.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 21, 2020

Headless/API-driven installs (like ours), which don't have a direct relationship between editor presentation and front-end presentation, are becoming increasingly common.

@chrisvanpatten is the classic preview button useful in a headless instance though? Asking this because I previously worked on a headless WP project that didn't use the preview at all, so I'm curious about how yours works 🤓

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 21, 2020

If the editor style opt in is easy to do, it seems like it has value regardless of the headless use case. Without an editor style, the responsive previews would be so inaccurate any way, that actual preview would be the best way to be sure.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 21, 2020

Without an editor style, the responsive previews would be so inaccurate any way

Yup, absolutely! The editor styles also need to be included in the stylesheets to be manipulated. I'll be looking at how to do that next.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Jan 21, 2020

Hi @tellthemachines, @noisysocks, I have been exploring the needed core work. Here is a summary of my findings. Please share any insights you may have.

The core may or may not use script concatenation. A user can disable it with CONCATENATE_SCRIPTS = false. It is also disabled if SCRIPT_DEBUG is true and other conditions.
The function script_concat_settings(); initates a global $concatenate_scripts that says if concatenation is on or off.

Problems with diving load-styles calls in multiple calls

Previously I suggested diving the load-styles.php in multiple calls, one that contains styles whose media queries we should rewrite and other that contains styles whose media queries we should not rewrite.

For example, if we want to isolate wp-block-editor, and wp-block-library as the styles we want to request a like (simplified):

http://yah.local/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=dashicons,admin-bar,buttons,media-views,editor-buttons,wp-components,wp-block-editor,wp-nux,wp-editor,wp-block-library&ver=5.4-alpha-20200116.104402

Would become:

http://test/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=dashicons,admin-bar,buttons,media-views,editor-buttons,wp-components,wp-nux,wp-editor&ver=5.4-alpha-20200116.104402

plus

http://test/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=wp-block-editor,wp-block-library&ver=5.4-alpha-20200116.104402

This simple division would not work because, for example, styles for wp-editor previously appeared after styles for wp-block-editor. In contrast, in the second version, wp-editor styles appear before. For the division to work, we would need to change the dependency management algorithm in https://github.com/WordPress/wordpress-develop/blob/3cdee7a7051b5dbdc87f36cb710bfa240f13278b/src/wp-includes/class.wp-dependencies.php#L144 to take into account not only the dependencies but also styles that cannot appear together.

For this simple case to keep the original order and isolate wp-block-editor, and wp-block-library (so we can rewrite only media queries inside this styles) we would need to do the following requests:

http://yah.local/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=dashicons,admin-bar,buttons,media-views,editor-buttons,wp-components&ver=5.4-alpha-20200116.104402

plus

http://yah.local/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=wp-block-editor&ver=5.4-alpha-20200116.104402

plus

http://yah.local/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=wp-nux,wp-editor&ver=5.4-alpha-20200116.104402
plus

http://yah.local/wp-admin/load-styles.php?c=1&dir=ltr&load[chunk_0]=wp-block-library&ver=5.4-alpha-20200116.104402

This logic involves many requests and goes against the reasons we concatenate the styles in the first place. In practice, the number of styles we want to isolate is bigger, so probably the number of requests is even more significant.

By exploring the code, it is also fair to assume everything was implemented with the assumption that the concatenation concatenates everything and there are no multiple concatenations. E.g we have a string that represents the style handles to concatenate separated by comma https://github.com/WordPress/wordpress-develop/blob/3cdee7a7051b5dbdc87f36cb710bfa240f13278b/src/wp-includes/class.wp-scripts.php#L59. A way to implement multiple calls to load-styles.php would be to allow $concat to be an array, but most plugins probably reasonably assume $concat is always a string and would break if it is an array.

So in summary, having multiple load-styles.php requests so we can separate the styles, we want to isolate is not a good idea because:

  • Goes against the whole objective of the concatenation of styles as we will have multiple load-styles.php calls (not only two).
  • It is complex to implement and will make the dependency management algorithm harder to understand.
  • May cause back-compatibly problems with plugins that assume there is only one concatenation group /load-styles.php request.

An alternative idea to isolate the styles whose media queries we transform in core

The alternative I have in mind is applying a straightforward change in load-styles.php https://github.com/jorgefilipecosta/wordpress-develop/blob/f0f94aef7ee93ef7388b8a04a0d67fafd0f419a7/src/wp-admin/load-styles.php#L72 that adds marks that allow JavaScript code to understand in what type of styles we are even if all of them are in the same request.
The mark cannot be a comment because the JS code can not access CSS comments (unless another request in ajax is made, which would be costly and would evolve a complex parsing).
So the marks I have in mind would be something like:
#start-section[wp-handle="wp-block-library"]{ display: none};
#end-section[wp-handle="wp-block-library"]{ display: none};

Then Gutenberg, as a setting would receive the URL of stylesheets or wp handles like "wp-block-library". For URL's Gutenberg would transform all the styles for handles Gutenberg would transform the styles in the load-styles.php call that appear between the rule #start-section[wp-handle="wp-block-library"]{ display: none}; and the rule #end-section[wp-handle="wp-block-library"]{ display: none};.
We may simplify and not add end mark and assume a section ends when another section starts or when the file ends.

This alternative allows us to keep all the logic and benefits of concatenation as it is now and involves adding these marks plus passing the correct setting object to the editor. This alternative seems more straightforward than multiple load-styles.php calls.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 21, 2020

@tellthemachines

is the classic preview button useful in a headless instance though? Asking this because I previously worked on a headless WP project that didn't use the preview at all, so I'm curious about how yours works 🤓

We actually don't use Gutenberg's Preview button for this reason, and in fact this is something else I wish we could easily disable and/or replace! We're using CSS to hide the "core" preview button, but I'm keenly aware that Gutenberg doesn't consider CSS selectors or DOM structure as part of the back-compat contract, and I'm bracing myself for the day it breaks 😅

Instead of core preview, we have our own button registered in one of the slotfill areas. It pushes the "dirty" post JSON into an internal graph database, then redirects to a special instance of our front-end which pulls in that data.

@tellthemachines tellthemachines requested review from nerrad and ntwb as code owners Jan 22, 2020
@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 22, 2020

Instead of core preview, we have our own button registered in one of the slotfill areas.

@chrisvanpatten yup, this sounds familiar 😁
In this case, would it be useful to allow for custom preview buttons in the dropdown, as suggested here?

It would definitely be good to introduce a non-hacky way of disabling the classic preview button too.

@jasmussen what might this component look like if we were to allow devs to fully customise which previews are available? (Not suggesting we do it as part of this PR, but perhaps we can mark it as an enhancement for later.)

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 22, 2020

@jorgefilipecosta thanks for investigating the style loading issue!

This simple division would not work because, for example, styles for wp-editor previously appeared after styles for wp-block-editor. In contrast, in the second version, wp-editor styles appear before.

How big a problem would this be? If reordering how the stylesheets load will break stuff, that's something we might want to fix anyway, as it probably means we have specificity issues in our CSS.

For URL's Gutenberg would transform all the styles for handles Gutenberg would transform the styles in the load-styles.php call that appear between the rule #start-section[wp-handle="wp-block-library"]{ display: none}; and the rule #end-section[wp-handle="wp-block-library"]{ display: none};.

Hmm, that would mean looping through all of that huge stylesheet every time we resize the editor. I guess we could do this as a last resort, but it would be much nicer if we could have a single solution that would work for both the plugin and core.

There's another problem I've just found though: the theme-defined editor styles are all loaded in a <style> tag at the bottom of the <body>, after we prefix them with .editor-styles-wrapper. If there are any media queries in these styles, we're unable to manipulate them in the same way we do the stylesheets. Not sure what we can do about this, but considering @ellatrix 's comment here perhaps it's not very important because editor styles will be going away at some point.

(Also, I noticed the wrapped editor styles <style> tag is being appended twice, is this a known issue?)

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 22, 2020

what might this component look like if we were to allow devs to fully customise which previews are available? (Not suggesting we do it as part of this PR, but perhaps we can mark it as an enhancement for later.)

That is an excellent question. I think the idea still needs to crystallize a little bit, and it will help to get this dropdown out there to help surface the best solutions. But here's one mockup of what it might look like:

Screenshot 2020-01-22 at 08 39 22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.