-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site Editor: add preview options component #21309
Changes from all commits
3021155
60b6e0f
afa373a
c3f4444
9505f16
e23b0de
55e55ad
3ba9adf
a0dc80b
3e3f130
1cd4b07
9b90de0
2376bfd
42e6399
09cfe0e
35b9bdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, I didn't want to duplicate that code, but also it's somewhat coupled with |
||
{ 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; | ||
|
@@ -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 { | ||
|
@@ -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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My aim here was to preserve the current way it looks in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah weird for me, we do have "hooks" folder in some packages. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( () => { | ||
|
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> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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 { | ||
|
@@ -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(), | ||
} ), | ||
[] | ||
); | ||
|
@@ -80,10 +84,7 @@ function Header( { onToggleInserter, isInserterOpen } ) { | |
forceIsSaving={ isSaving } | ||
/> | ||
) } | ||
<PreviewOptions | ||
forceIsAutosaveable={ hasActiveMetaboxes } | ||
forcePreviewLink={ isSaving ? null : undefined } | ||
/> | ||
<DevicePreview /> | ||
<PostPreviewButton | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @tellthemachines, I'll look into bringing it back. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 } | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
andedit-site
context. I didn't want to place them ineditor
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?There was a problem hiding this comment.
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 inedit-post
andedit-site
separately in 3e3f130.I considered using local state for it, but since both
PreviewOptions
anduseResizeCanvas
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.