Site Editor: add preview options component by vindl · Pull Request #21309 · WordPress/gutenberg

vindl · 2020-04-01T03:06:50Z

Description

Integrates the PreviewOptions component from the post editor in site editor context.

I extracted the UseResizeCanvas and PreviewOptions component to block-editor package and it now only contains the devices menu, while the Preview in new tab can be optionally added in contexts where it makes sense, like the post editor.

Part of #19253.

How has this been tested?

Tested with Twenty Twenty theme.

Verify that this is not causing regressions in the existing post editor functionality.
Test the preview options in site editor.

Screenshots

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

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.

github-actions · 2020-04-01T03:13:29Z

Size Change: -152 B (0%)

Total Size: 845 kB

Filename	Size	Change
`build/block-editor/index.js`	106 kB	+622 B (0%)
`build/block-editor/style-rtl.css`	10.2 kB	+28 B (0%)
`build/block-editor/style.css`	10.2 kB	+34 B (0%)
`build/block-library/editor-rtl.css`	7.05 kB	-81 B (1%)
`build/block-library/editor.css`	7.05 kB	-78 B (1%)
`build/block-library/index.js`	112 kB	+1 B
`build/block-library/style-rtl.css`	7.14 kB	-46 B (0%)
`build/block-library/style.css`	7.14 kB	-49 B (0%)
`build/components/index.js`	198 kB	-6 B (0%)
`build/data-controls/index.js`	1.29 kB	+40 B (3%)
`build/edit-post/index.js`	27.7 kB	-450 B (1%)
`build/edit-post/style-rtl.css`	12.3 kB	-187 B (1%)
`build/edit-post/style.css`	12.3 kB	-186 B (1%)
`build/edit-site/index.js`	11 kB	+179 B (1%)
`build/edit-site/style-rtl.css`	5.26 kB	+14 B (0%)
`build/edit-site/style.css`	5.26 kB	+14 B (0%)
`build/edit-widgets/index.js`	8.33 kB	-1 B
`build/editor/index.js`	43.3 kB	+1 B
`build/warning/index.js`	1.14 kB	-1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.62 kB	0 B
`build/api-fetch/index.js`	4.08 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.24 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	761 B	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.7 kB	0 B
`build/components/style-rtl.css`	16.9 kB	0 B
`build/components/style.css`	16.9 kB	0 B
`build/compose/index.js`	6.66 kB	0 B
`build/core-data/index.js`	11.4 kB	0 B
`build/data/index.js`	8.43 kB	0 B
`build/date/index.js`	5.47 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.1 kB	0 B
`build/edit-navigation/index.js`	3.54 kB	0 B
`build/edit-navigation/style-rtl.css`	485 B	0 B
`build/edit-navigation/style.css`	485 B	0 B
`build/edit-widgets/style-rtl.css`	5 kB	0 B
`build/edit-widgets/style.css`	5 kB	0 B
`build/editor/editor-styles-rtl.css`	428 B	0 B
`build/editor/editor-styles.css`	431 B	0 B
`build/editor/style-rtl.css`	3.27 kB	0 B
`build/editor/style.css`	3.27 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.32 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/keyboard-shortcuts/index.js`	2.51 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.12 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	5.29 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.67 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.8 kB	0 B
`build/server-side-render/index.js`	2.67 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

Addison-Stavlo

(edit: whooops! I missed the in progress tag and just starting going with the review request 😆 )

There are 2 things that catch my eye testing:

These changes seem to make the 'Preview in a new tab' link disabled in places they used to be usable. This example is taken from the standard editor for one of my pages:

Looking at the code, I'm not quite sure what would cause this.
I'm not sure if we are missing some necessary styles in the site editor to highlight the previews, but in the standard editor we see the gray background:

This gray background isn't apparent in the site editor, which makes it difficult to determine where the preview starts:

I can't speak to what packages which things belong in, but these changes seem to make sense so far.

vindl · 2020-04-02T02:47:26Z

(edit: whooops! I missed the in progress tag and just starting going with the review request 😆 )

That's fine, I'm mostly looking for reviews to see if the approach makes sense before diving into the more details.

These changes seem to make the 'Preview in a new tab' link disabled in places they used to be usable.

I noticed that too and at this point I'm not sure either.

I'm not sure if we are missing some necessary styles in the site editor to highlight the previews, but in the standard editor we see the gray background:

I don't think that was the case initially in my testing but I can reproduce it now. Maybe related to some of the newer changes that I picked up with the rebase.

vindl · 2020-04-16T00:45:46Z

After chatting about this with @youknowriad I changed the approach and extracted the relevant components and moved them to block-editor package.

These changes seem to make the 'Preview in a new tab' link disabled in places they used to be usable. This example is taken from the standard editor for one of my pages:

@Addison-Stavlo this should be resolved with latest changes.

I'm not sure if we are missing some necessary styles in the site editor to highlight the previews, but in the standard editor we see the gray background:

The gray background has also been added now.

vindl · 2020-04-16T00:48:01Z

packages/edit-post/src/components/header/index.js

This is the old preview button and I couldn't see the use for it here since it seems to be hidden. It would be nice if someone with more experience with this code could check it out and make sure that this removal won't break something.

The old preview button is shown on small screens, as indicated by the CSS you removed above, which initially sets editor-post-preview__dropdown to display: none, and then sets editor-post-preview to display: none on the small breakpoint.

The reason for this is that the resizable previews are still not optimised to work on mobile devices, so we opted to only show them on larger screens.

Thanks @tellthemachines, I'll look into bringing it back.

@tellthemachines I brought the old preview button back along with the required styles in dcbaf85.

tellthemachines

Thanks for working on this! I left a couple of comments below. Also, you'll need to pop into the e2e tests and change the classnames used for targeting this component to match your changes 🙂

tellthemachines · 2020-04-16T01:58:52Z

packages/edit-post/src/components/header/index.js

The old preview button is shown on small screens, as indicated by the CSS you removed above, which initially sets editor-post-preview__dropdown to display: none, and then sets editor-post-preview to display: none on the small breakpoint.

The reason for this is that the resizable previews are still not optimised to work on mobile devices, so we opted to only show them on larger screens.

tellthemachines · 2020-04-16T02:13:27Z

packages/block-editor/src/style.scss

 @import "./components/media-placeholder/style.scss";
 @import "./components/multi-selection-inspector/style.scss";
 @import "./components/plain-text/style.scss";
+@import "./components/preview-options/style.scss";


This should be moved to beneath line 55, under the marker for resizable styles, so that its contents aren't impacted by the device preview style manipulation.

Updated in dcbaf85.

simison · 2020-04-16T06:53:56Z

packages/block-editor/src/components/preview-options/index.js

+import { __ } from '@wordpress/i18n';
+import { check } from '@wordpress/icons';
+
+const downArrow = (


Is this same as https://github.com/WordPress/gutenberg/blob/master/packages/icons/src/library/arrow-down.js ?

No, that one looks different, you can check it out here (also the path value was different in that code compared to what we have here).

So this is chevronDown right?

So this is chevronDown right?

Oh right, missed that one. Updated in 42e6399.

vindl · 2020-04-16T13:07:57Z

Also, you'll need to pop into the e2e tests and change the classnames used for targeting this component to match your changes 🙂

@tellthemachines the e2e tests are fixed now. This should be ready for another pass. :)

Addison-Stavlo · 2020-04-16T13:17:20Z

Trying this now, im having the issue that the entire background for the site editor has turned gray.

vindl · 2020-04-16T14:55:12Z

@Addison-Stavlo I can't reproduce this, the background is the same for me as indicated in the screenshot in the summary.

youknowriad · 2020-04-23T09:07:06Z

packages/block-editor/src/components/preview-options/index.js

+						>
+							{ __( 'Mobile' ) }
+						</MenuItem>
+					</MenuGroup>


I wonder this component should just render the MenuGroup and not the whole dropdown, I guess you did it this way because you wanted to share dropdown code but it does seem like it reduces the semantic value of the component a bit. I don't see this as a blocker or anything for now, just something to think about.

I guess you did it this way because you wanted to share dropdown code but it does seem like it reduces the semantic value of the component a bit.

Right, I didn't want to duplicate that code, but also it's somewhat coupled with useResizeCanvas now in terms of what device strings it sets in the store. Until that is reworked somehow, I think it makes sense to keep it as is.

youknowriad · 2020-04-23T09:08:22Z

packages/block-editor/src/components/preview-options/style.scss

+	.block-editor-post-preview__dropdown {
 		display: flex;
 	}
 }


This component seem to have a lot of custom styles, It looks very similar to "More Menus" or "Block Settings Menus" so I wonder why it needs all these styles and can't just rely on the default UI components styles. Maybe our UI components styles are not well thought in this situation.

My aim here was to preserve the current way it looks in edit-post completely. I think we could consider removing these, but I also think it would be better to tackle that in a separate PR.

Though some of these styles exist, they are currently component-specific: that's the case with the "more menu" buttons and their selected state. The original PR to implement this was already huge, so I optimistically 😁 thought to tackle those improvements later. Other styles, such as the dropdown toggle, don't exist anywhere else afaik. Agree this should be addressed in a separate PR.

youknowriad · 2020-04-23T09:09:33Z

packages/block-editor/src/components/resize-canvas/index.js

-		return select( 'core/edit-post' ).__experimentalGetPreviewDeviceType();
-	}, [] );
-
+export default function useResizeCanvas( deviceType ) {


Should this be use-resize-canvas.js in a "hooks" folder instead of "components"?

I'm not sure but I can look into it more. I placed it in components since that's where it was in edit-post.

yeah weird for me, we do have "hooks" folder in some packages.

I tried to rework it here as suggested 9b90de0, not sure if it's the right way to go about it though. :)

youknowriad · 2020-04-23T09:10:46Z

packages/edit-post/src/components/header/index.js

+				>
+					<MenuGroup>
+						<div className="edit-post-header-preview__grouping-external">
+							<PostPreviewButton


Nit: Potentially, we could extract this to a small component somewhere ini edit-post to avoid having to add selectors... specific to this component here.

I think this would make sense for the whole PreviewOptions section here too. 🤔

Refactored in 2376bfd.

youknowriad

Left a few questions but this is looking good for me.

youknowriad · 2020-04-23T16:36:36Z

packages/block-editor/src/hooks/use-resize-canvas.js

 	return contentInlineStyles( deviceType );
 }
+
+addFilter(


Is this a new filter, did we have it before?

Yes, it's a new filter.

We've been avoiding adding new filters in Gutenberg as we figured that filters and hooks are not the best API for extensibility in JS.

Could you clarify why this filter is needed and do you think we should have it added in its own PR instead? We could discuss alternatives... for your use cases.

I don't think it's needed, it's just that I noticed that that's how hooks are usually organized/used (haven't worked with them so far in Gutenberg), so I figured that a request to move this to hooks folder would imply a filter.

Ohhh, I guess we have a naming conflicts (react hooks vs WP hooks) 🤔

But now I realize that my assumption has been wrong 😅. I pushed a revert for that in 09cfe0e.

This reverts commit 9b90de0.

tellthemachines

This is looking good!

For some weird reason, the popover on the site editor is now opening towards the right, whereas on the post editor it still opens to the left. Might be worth setting an explicit position value in the Dropdown component so it is consistent in both places. (How it looks on post editor seems less ambiguous as to what its parent is.)

tellthemachines · 2020-04-24T03:33:27Z

packages/block-editor/src/components/preview-options/style.scss

+	.block-editor-post-preview__dropdown {
 		display: flex;
 	}
 }


Though some of these styles exist, they are currently component-specific: that's the case with the "more menu" buttons and their selected state. The original PR to implement this was already huge, so I optimistically 😁 thought to tackle those improvements later. Other styles, such as the dropdown toggle, don't exist anywhere else afaik. Agree this should be addressed in a separate PR.

youknowriad

Nice

vindl · 2020-04-24T11:55:35Z

For some weird reason, the popover on the site editor is now opening towards the right, whereas on the post editor it still opens to the left. Might be worth setting an explicit position value in the Dropdown component so it is consistent in both places. (How it looks on post editor seems less ambiguous as to what its parent is.)

Good catch @tellthemachines, I updated this in 35b9bdf.

vindl added the [Feature] Full Site Editing label Apr 1, 2020

vindl requested review from ellatrix and youknowriad as code owners April 1, 2020 03:06

vindl self-assigned this Apr 1, 2020

vindl requested review from Addison-Stavlo and noahtallen April 1, 2020 03:07

vindl force-pushed the add/site-editor-preview branch from e975a96 to ad6e08f Compare April 1, 2020 03:10

vindl added the [Status] In Progress Tracking issues with work in progress label Apr 1, 2020

Addison-Stavlo reviewed Apr 2, 2020

View reviewed changes

vindl requested review from jorgefilipecosta, mtias and tellthemachines April 2, 2020 14:52

vindl force-pushed the add/site-editor-preview branch 2 times, most recently from e21ee4b to 4921851 Compare April 15, 2020 22:13

vindl removed the [Status] In Progress Tracking issues with work in progress label Apr 16, 2020

vindl commented Apr 16, 2020

View reviewed changes

vindl force-pushed the add/site-editor-preview branch from a25e1ad to 74e33ab Compare April 16, 2020 00:52

tellthemachines requested changes Apr 16, 2020

View reviewed changes

simison reviewed Apr 16, 2020

View reviewed changes

vindl requested review from ajitbohra, nerrad, ntwb and talldan as code owners April 16, 2020 11:44

vindl force-pushed the add/site-editor-preview branch from 7b03bc2 to dcbaf85 Compare April 16, 2020 12:10

Fix e2e tests

a0dc80b

vindl force-pushed the add/site-editor-preview branch 2 times, most recently from dc50eb9 to 4fd6036 Compare April 22, 2020 23:16

Move device type store out of block-editor package

3e3f130

vindl force-pushed the add/site-editor-preview branch from 4fd6036 to 3e3f130 Compare April 22, 2020 23:22

youknowriad mentioned this pull request Apr 23, 2020

Full Site Editing Breakdown #20791

Closed

53 tasks

Fix e2e tests

1cd4b07

vindl force-pushed the add/site-editor-preview branch from 1f63e54 to 1cd4b07 Compare April 23, 2020 04:02

youknowriad reviewed Apr 23, 2020

View reviewed changes

Move useResizeCanvas to hooks

9b90de0

youknowriad reviewed Apr 23, 2020

View reviewed changes

vindl added 3 commits April 23, 2020 18:48

Isolate edit-post device preview chunk as a separate component

2376bfd

Replace downArrow with chevronDown icon

42e6399

Revert "Move useResizeCanvas to hooks"

09cfe0e

This reverts commit 9b90de0.

tellthemachines reviewed Apr 24, 2020

View reviewed changes

youknowriad approved these changes Apr 24, 2020

View reviewed changes

vindl requested a review from tellthemachines April 24, 2020 11:39

Set dropdown position to bottom left

35b9bdf

tellthemachines approved these changes Apr 24, 2020

View reviewed changes

vindl merged commit f68d50a into master Apr 24, 2020

vindl deleted the add/site-editor-preview branch April 24, 2020 12:27

github-actions bot added this to the Gutenberg 8.0 milestone Apr 24, 2020

vindl mentioned this pull request Apr 24, 2020

Site Editing: UI Parity #21245

Closed

17 tasks

andrewserong mentioned this pull request Apr 28, 2020

Gutenberg v8.0.0: duplicate Preview links appearing in the header Automattic/wp-calypso#41519

Closed

kevin940726 mentioned this pull request Oct 19, 2021

Add resizer to template part focus mode #35728

Merged

7 tasks

Conversation

vindl commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

github-actions bot commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Addison-Stavlo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vindl commented Apr 2, 2020

Uh oh!

vindl commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tellthemachines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vindl Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vindl commented Apr 16, 2020

Uh oh!

Addison-Stavlo commented Apr 16, 2020

Uh oh!

vindl commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vindl Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vindl commented Apr 1, 2020 •

edited

Loading

github-actions bot commented Apr 1, 2020 •

edited

Loading

Addison-Stavlo left a comment •

edited

Loading

vindl Apr 23, 2020 •

edited

Loading

vindl Apr 23, 2020 •

edited

Loading

youknowriad Apr 23, 2020 •

edited

Loading