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

Site Editor: add preview options component #21309

Merged
merged 16 commits into from
Apr 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ export { default as withColorContext } from './color-palette/with-color-context'

export { default as __experimentalBlockSettingsMenuFirstItem } from './block-settings-menu/block-settings-menu-first-item';
export { default as __experimentalInserterMenuExtension } from './inserter-menu-extension';
export { default as __experimentalPreviewOptions } from './preview-options';
export { default as __experimentalUseResizeCanvas } from './resize-canvas';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for including these components and their state in the block-editor package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just looking for a good place for them so that they could be used both in edit-post and edit-site context. I didn't want to place them in editor since it depends on wp and post, and this could be useful outside of that context too. Do you think they should be placed in some other package?

Copy link
Member Author

@vindl vindl Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad based on our conversation I removed the required state from block-editor component and placed it in edit-post and edit-site separately in 3e3f130.

I considered using local state for it, but since both PreviewOptions and useResizeCanvas rely on it, and these are at different nesting levels and parts of the editor, this seemed cleaner that propagating this through props or using context.

export { default as BlockInspector } from './block-inspector';
export { default as BlockList } from './block-list';
export { Block as __experimentalBlock } from './block-list/block-wrapper';
Expand Down
70 changes: 70 additions & 0 deletions packages/block-editor/src/components/preview-options/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { Button, Dropdown, MenuGroup, MenuItem } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { Icon, check, chevronDown } from '@wordpress/icons';

export default function PreviewOptions( {
children,
className,
isEnabled = true,
deviceType,
setDeviceType,
} ) {
return (
<Dropdown
className="block-editor-post-preview__dropdown"
contentClassName={ classnames(
className,
'block-editor-post-preview__dropdown-content'
) }
popoverProps={ { role: 'menu' } }
position="bottom left"
renderToggle={ ( { isOpen, onToggle } ) => (
<Button
onClick={ onToggle }
className="block-editor-post-preview__button-toggle"
aria-expanded={ isOpen }
disabled={ ! isEnabled }
>
{ __( 'Preview' ) }
<Icon icon={ chevronDown } />
</Button>
) }
renderContent={ () => (
<>
<MenuGroup>
<MenuItem
className="block-editor-post-preview__button-resize"
onClick={ () => setDeviceType( 'Desktop' ) }
icon={ deviceType === 'Desktop' && check }
>
{ __( 'Desktop' ) }
</MenuItem>
<MenuItem
className="block-editor-post-preview__button-resize"
onClick={ () => setDeviceType( 'Tablet' ) }
icon={ deviceType === 'Tablet' && check }
>
{ __( 'Tablet' ) }
</MenuItem>
<MenuItem
className="block-editor-post-preview__button-resize"
onClick={ () => setDeviceType( 'Mobile' ) }
icon={ deviceType === 'Mobile' && check }
>
{ __( 'Mobile' ) }
</MenuItem>
</MenuGroup>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ children }
</>
) }
/>
);
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
.editor-post-preview__dropdown {
.block-editor-post-preview__dropdown {
display: none;
margin-right: $grid-unit-15;
padding: 0;
}

.editor-post-preview__button-toggle {
.block-editor-post-preview__button-toggle {
display: flex;
justify-content: space-between;
padding: 0 $grid-unit-10 0 $grid-unit-15;
Expand All @@ -18,18 +18,17 @@
}
}

.editor-post-preview__button-resize.editor-post-preview__button-resize {
.block-editor-post-preview__button-resize.block-editor-post-preview__button-resize {
padding-left: $button-size-small + $grid-unit-10 + $grid-unit-10;

&.has-icon {
padding-left: $grid-unit-10;
}
}

.editor-post-preview__dropdown-content {
.block-editor-post-preview__dropdown-content {
.components-popover__content {
overflow-y: visible;
padding-bottom: 0;
}

.components-menu-group + .components-menu-group {
Expand All @@ -40,29 +39,12 @@
}
}

.editor-post-preview__grouping-external {
display: flex;
position: relative;
}

.editor-post-preview__button-external {
padding-left: $grid-unit-10;
margin-right: auto;
width: 100%;
display: flex;
justify-content: flex-start;

svg {
margin-right: $grid-unit-10;
}
}

@include break-small() {
.editor-post-preview {
display: none;
}

.editor-post-preview__dropdown {
.block-editor-post-preview__dropdown {
display: flex;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@vindl vindl Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
/**
* WordPress dependencies
*/
import { useSimulatedMediaQuery } from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';
import { useEffect, useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import { default as useSimulatedMediaQuery } from '../../components/use-simulated-media-query';

/**
* Function to resize the editor window.
*
* @param {string} deviceType Used for determining the size of the container (e.g. Desktop, Tablet, Mobile)
*
* @return {Object} Inline styles to be added to resizable container.
*/
export function useResizeCanvas() {
const deviceType = useSelect( ( select ) => {
return select( 'core/edit-post' ).__experimentalGetPreviewDeviceType();
}, [] );

export default function useResizeCanvas( deviceType ) {
Copy link
Contributor

@youknowriad youknowriad Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const [ actualWidth, updateActualWidth ] = useState( window.innerWidth );

useEffect( () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,4 @@

@import "./components/block-toolbar/style.scss";
@import "./components/inserter/style.scss";

@import "./components/preview-options/style.scss";
6 changes: 3 additions & 3 deletions packages/e2e-tests/specs/editor/plugins/block-context.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import {
async function openPreviewPage( editorPage ) {
let openTabs = await browser.pages();
const expectedTabsCount = openTabs.length + 1;
await editorPage.click( '.editor-post-preview__button-toggle' );
await editorPage.waitFor( '.editor-post-preview__button-external' );
await editorPage.click( '.editor-post-preview__button-external' );
await editorPage.click( '.block-editor-post-preview__button-toggle' );
await editorPage.waitFor( '.edit-post-header-preview__button-external' );
await editorPage.click( '.edit-post-header-preview__button-external' );

// Wait for the new tab to open.
while ( openTabs.length < expectedTabsCount ) {
Expand Down
18 changes: 9 additions & 9 deletions packages/e2e-tests/specs/editor/various/preview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import {
async function openPreviewPage( editorPage ) {
let openTabs = await browser.pages();
const expectedTabsCount = openTabs.length + 1;
await editorPage.click( '.editor-post-preview__button-toggle' );
await editorPage.waitFor( '.editor-post-preview__button-external' );
await editorPage.click( '.editor-post-preview__button-external' );
await editorPage.click( '.block-editor-post-preview__button-toggle' );
await editorPage.waitFor( '.edit-post-header-preview__button-external' );
await editorPage.click( '.edit-post-header-preview__button-external' );

// Wait for the new tab to open.
while ( openTabs.length < expectedTabsCount ) {
Expand All @@ -49,9 +49,9 @@ async function openPreviewPage( editorPage ) {
* @return {Promise} Promise resolving once selector is visible on page.
*/
async function waitForPreviewDropdownOpen( editorPage ) {
await editorPage.click( '.editor-post-preview__button-toggle' );
await editorPage.click( '.block-editor-post-preview__button-toggle' );
return editorPage.waitForSelector(
'.editor-post-preview__button-external'
'.edit-post-header-preview__button-external'
);
}

Expand All @@ -64,7 +64,7 @@ async function waitForPreviewDropdownOpen( editorPage ) {
* @return {Promise} Promise resolving once navigation completes.
*/
async function waitForPreviewNavigation( previewPage ) {
await page.click( '.editor-post-preview__button-external' );
await page.click( '.edit-post-header-preview__button-external' );
return previewPage.waitForNavigation();
}

Expand Down Expand Up @@ -115,7 +115,7 @@ describe( 'Preview', () => {

// Disabled until content present.
const isPreviewDisabled = await editorPage.$$eval(
'.editor-post-preview__button-toggle:not( :disabled ):not( [aria-disabled="true"] )',
'.block-editor-post-preview__button-toggle:not( :disabled ):not( [aria-disabled="true"] )',
( enabledButtons ) => ! enabledButtons.length
);
expect( isPreviewDisabled ).toBe( true );
Expand Down Expand Up @@ -342,10 +342,10 @@ describe( 'Preview with private custom post type', () => {
await page.keyboard.press( 'Tab' );

// Open the preview menu.
await page.click( '.editor-post-preview__button-toggle' );
await page.click( '.block-editor-post-preview__button-toggle' );

const previewDropdownContents = await page.$(
'.editor-post-preview__dropdown-content'
'.block-editor-post-preview__dropdown-content'
);

// Expect the Preview Externally link not to be present.
Expand Down
58 changes: 58 additions & 0 deletions packages/edit-post/src/components/device-preview/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* WordPress dependencies
*/
import { Icon, MenuGroup } from '@wordpress/components';
import { PostPreviewButton } from '@wordpress/editor';
import { external } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';
import { __experimentalPreviewOptions as PreviewOptions } from '@wordpress/block-editor';
import { useDispatch, useSelect } from '@wordpress/data';

export default function DevicePreview() {
const {
hasActiveMetaboxes,
isPostSaveable,
isSaving,
deviceType,
} = useSelect(
( select ) => ( {
hasActiveMetaboxes: select( 'core/edit-post' ).hasMetaBoxes(),
isSaving: select( 'core/edit-post' ).isSavingMetaBoxes(),
isPostSaveable: select( 'core/editor' ).isEditedPostSaveable(),
deviceType: select(
'core/edit-post'
).__experimentalGetPreviewDeviceType(),
} ),
[]
);
const {
__experimentalSetPreviewDeviceType: setPreviewDeviceType,
} = useDispatch( 'core/edit-post' );

return (
<PreviewOptions
isEnabled={ isPostSaveable }
className="edit-post-post-preview-dropdown"
deviceType={ deviceType }
setDeviceType={ setPreviewDeviceType }
>
<MenuGroup>
<div className="edit-post-header-preview__grouping-external">
<PostPreviewButton
className={
'edit-post-header-preview__button-external'
}
forceIsAutosaveable={ hasActiveMetaboxes }
forcePreviewLink={ isSaving ? null : undefined }
textContent={
<>
<Icon icon={ external } />
{ __( 'Preview in new tab' ) }
</>
}
/>
</div>
</MenuGroup>
</PreviewOptions>
);
}
13 changes: 7 additions & 6 deletions packages/edit-post/src/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import { __ } from '@wordpress/i18n';
import { Button } from '@wordpress/components';
import { PostPreviewButton, PostSavedState } from '@wordpress/editor';
import { PostSavedState, PostPreviewButton } from '@wordpress/editor';
import { useSelect, useDispatch } from '@wordpress/data';
import { cog } from '@wordpress/icons';
import { PinnedItems, AdminMenuToggle } from '@wordpress/interface';
Expand All @@ -14,7 +14,7 @@ import { PinnedItems, AdminMenuToggle } from '@wordpress/interface';
import HeaderToolbar from './header-toolbar';
import MoreMenu from './more-menu';
import PostPublishButtonOrToggle from './post-publish-button-or-toggle';
import PreviewOptions from '../preview-options';
import { default as DevicePreview } from '../device-preview';

function Header( { onToggleInserter, isInserterOpen } ) {
const {
Expand All @@ -40,9 +40,13 @@ function Header( { onToggleInserter, isInserterOpen } ) {
isSaving: select( 'core/edit-post' ).isSavingMetaBoxes(),
getBlockSelectionStart: select( 'core/block-editor' )
.getBlockSelectionStart,
isPostSaveable: select( 'core/editor' ).isEditedPostSaveable(),
isFullscreenActive: select( 'core/edit-post' ).isFeatureActive(
'fullscreenMode'
),
deviceType: select(
'core/edit-post'
).__experimentalGetPreviewDeviceType(),
} ),
[]
);
Expand Down Expand Up @@ -80,10 +84,7 @@ function Header( { onToggleInserter, isInserterOpen } ) {
forceIsSaving={ isSaving }
/>
) }
<PreviewOptions
forceIsAutosaveable={ hasActiveMetaboxes }
forcePreviewLink={ isSaving ? null : undefined }
/>
<DevicePreview />
<PostPreviewButton
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

forceIsAutosaveable={ hasActiveMetaboxes }
forcePreviewLink={ isSaving ? null : undefined }
Expand Down
27 changes: 26 additions & 1 deletion packages/edit-post/src/components/header/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
.editor-post-saved-state,
.components-button.editor-post-switch-to-draft,
.components-button.editor-post-preview,
.components-button.editor-post-preview__dropdown,
.components-button.block-editor-post-preview__dropdown,
.components-button.editor-post-publish-button,
.components-button.editor-post-publish-panel__toggle {
padding: 0 6px;
Expand All @@ -82,3 +82,28 @@
}
}
}

.edit-post-header-preview__grouping-external {
display: flex;
position: relative;
padding-bottom: 0;
}

.edit-post-header-preview__button-external {
padding-left: $grid-unit-10;

margin-right: auto;
width: 100%;
display: flex;
justify-content: flex-start;

svg {
margin-right: $grid-unit-10;
}
}

.edit-post-post-preview-dropdown {
.components-popover__content {
padding-bottom: 0;
}
}
Loading