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

Merged
merged 39 commits into from
Feb 11, 2020
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
dfaa1b7
Resizable editor experiment
tellthemachines Dec 12, 2019
2720b04
Add Jorge's simulate media query component
tellthemachines Dec 18, 2019
6f0ca43
Integrate Jorge's solution with resizable editor
tellthemachines Dec 18, 2019
600a9ff
docs update
tellthemachines Dec 18, 2019
122ccee
Add stylesheet list as editor setting.
tellthemachines Jan 13, 2020
75f7d28
Stage management for editing canvas device type.
tellthemachines Jan 14, 2020
812e31a
Fix IE incompatibility.
tellthemachines Jan 15, 2020
39370bc
Dropdown styling.
tellthemachines Jan 15, 2020
a9f5d54
Fixed styling bugs
tellthemachines Jan 16, 2020
21f4a16
Classic preview button for mobile.
tellthemachines Jan 16, 2020
63bbfa5
Accurate styles on window resize.
tellthemachines Jan 16, 2020
aaec420
Fix preview button unit tests.
tellthemachines Jan 16, 2020
ad4c669
Docs updates.
tellthemachines Jan 17, 2020
86484be
Min heights and width adjustment.
tellthemachines Jan 17, 2020
6dd6e75
Address styling issues.
tellthemachines Jan 20, 2020
3f70996
Attempt to fix block inserter menu.
tellthemachines Jan 20, 2020
e8e1d92
Address review feedback.
tellthemachines Jan 21, 2020
f56765e
Remove unused styles.
tellthemachines Jan 21, 2020
93e6e45
Fix e2e tests.
tellthemachines Jan 22, 2020
1e446e0
Remove non-existent stylesheet
tellthemachines Jan 22, 2020
860670d
Add support for parsing concatenated stylesheets
tellthemachines Jan 30, 2020
4ecef11
Fix lint error.
tellthemachines Jan 30, 2020
d23bac2
Address styling feedback
tellthemachines Jan 30, 2020
9c42b28
Apply style change on focus.
tellthemachines Jan 30, 2020
3544005
Styling tweaks.
tellthemachines Jan 30, 2020
a6668be
Parse all stylesheets from same domain
tellthemachines Feb 5, 2020
51e578b
lint
tellthemachines Feb 5, 2020
c28e1e6
Lint.
tellthemachines Feb 6, 2020
231a89f
Move canvas resizing to visual editor
tellthemachines Feb 9, 2020
718f246
Lint and style fixes
tellthemachines Feb 9, 2020
f832790
Merge branch 'master' into try/resizable-editor
tellthemachines Feb 10, 2020
7d2bf12
Make preview dropdown edit-post component.
tellthemachines Feb 10, 2020
50370fc
Styling adjustments.
tellthemachines Feb 10, 2020
61471d0
Add inline docs and pass marker as parameter.
tellthemachines Feb 10, 2020
0696f6a
Rename selector.
tellthemachines Feb 10, 2020
dd53ad9
Miscellaneous styling fixes
tellthemachines Feb 10, 2020
aedf6bc
Early return
tellthemachines Feb 10, 2020
3356171
Button height adjustment.
tellthemachines Feb 11, 2020
ba531c4
Update docs.
tellthemachines Feb 11, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,18 @@ _Returns_

- `Array`: ids of top-level and descendant blocks.

<a name="getDeviceType" href="#getDeviceType">#</a> **getDeviceType**
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be named similarily to the setter? getPreviewDeviceType?


Returns the current editing canvas device type.

_Parameters_

- _state_ `Object`: Global application state.

_Returns_

- `string`: Device type.

<a name="getFirstMultiSelectedBlockClientId" href="#getFirstMultiSelectedBlockClientId">#</a> **getFirstMultiSelectedBlockClientId**

Returns the client ID of the first block in the multi-selection set, or null
Expand Down Expand Up @@ -1197,6 +1209,18 @@ _Parameters_

- _isNavigationMode_ `string`: Enable/Disable navigation mode.

<a name="setPreviewDeviceType" href="#setPreviewDeviceType">#</a> **setPreviewDeviceType**

Returns an action object used to toggle the width of the editing canvas

_Parameters_

- _deviceType_ `string`:

_Returns_

- `Object`: Action object.

<a name="setTemplateValidity" href="#setTemplateValidity">#</a> **setTemplateValidity**

Returns an action object resetting the template validity.
Expand Down
1 change: 1 addition & 0 deletions lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -677,3 +677,4 @@ function gutenberg_extend_block_editor_preload_paths( $preload_paths, $post ) {
return $preload_paths;
}
add_filter( 'block_editor_preload_paths', 'gutenberg_extend_block_editor_preload_paths', 10, 2 );

6 changes: 6 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions packages/block-editor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,15 @@ _Related_

Undocumented declaration.

<a name="useSimulatedMediaQuery" href="#useSimulatedMediaQuery">#</a> **useSimulatedMediaQuery**

Function that manipulates media queries from stylesheets to simulate a given viewport width.

_Parameters_

- _partialPaths_ `Array`: Paths of stylesheets to manipulate.
- _width_ `number`: Viewport width to simulate.

<a name="Warning" href="#Warning">#</a> **Warning**

Undocumented declaration.
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"@wordpress/viewport": "file:../viewport",
"@wordpress/wordcount": "file:../wordcount",
"classnames": "^2.2.5",
"css-mediaquery": "^0.1.2",
"diff": "^3.5.0",
"dom-scroll-into-view": "^1.2.1",
"inherits": "^2.0.3",
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,4 @@ export { default as WritingFlow } from './writing-flow';
*/

export { default as BlockEditorProvider } from './provider';
export { default as useSimulatedMediaQuery } from './simulate-media-query';
3 changes: 2 additions & 1 deletion packages/block-editor/src/components/inserter/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ $block-inserter-search-height: 38px;
height: 100%;
display: flex;
width: auto;
min-width: 400px;
Copy link
Member

Choose a reason for hiding this comment

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

What about viewports which are narrower than 400px?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, that needs to be in a media query.

position: relative;

@include break-medium {
width: 400px;
position: relative;

&.has-help-panel {
width: 700px;
Expand Down
126 changes: 126 additions & 0 deletions packages/block-editor/src/components/simulate-media-query/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
Copy link
Member

@aduth aduth Feb 14, 2020

Choose a reason for hiding this comment

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

I'd expect if the default export of this file is useSimulatedMediaQuery, the folder ought to be named accordingly, i.e. use-simulated-media-query. Both for consistency, but also because it helps discoverability of the hook source when working backward from the known export.

Prior art:

* External dependencies
*/
import { filter, get } from 'lodash';
import { match } from 'css-mediaquery';

/**
* WordPress dependencies
*/
import { useEffect } from '@wordpress/element';

const ENABLED_MEDIA_QUERY = '(min-width:0px)';
const DISABLED_MEDIA_QUERY = '(min-width:999999px)';
Copy link
Member

Choose a reason for hiding this comment

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

Would someone with a setup like this mess us up? 🤭

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, I guess we could add a few more digits. But I'd rather leave it as is just because I'd love to see the screenshot for that bug report 😂

Copy link
Member

Choose a reason for hiding this comment

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

I concur.


const VALID_MEDIA_QUERY_REGEX = /\((min|max)-width:[^\(]*?\)/g;

function getStyleSheetsThatMatchHostname() {
Copy link
Member

Choose a reason for hiding this comment

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

Any reasons to return early from this function if window is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. This code shouldn't ever run on the server side, but better safe than sorry I guess 😅

return filter(
get( window, [ 'document', 'styleSheets' ], [] ),
( styleSheet ) => {
return (
styleSheet.href &&
styleSheet.href.includes( window.location.hostname )
Copy link
Contributor

@mikeselander mikeselander Feb 18, 2020

Choose a reason for hiding this comment

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

There's a couple of issues with this simplistic of logic:

  1. I believe this excludes any styles passed in via add_editor_style as they're added in an inline style element.
  2. We should additionally be excluding any print stylesheets as they're not useful to this logic.
  3. We should also be excluding most plugin stylesheets from this list as they don't have any block-list related styles. This probably seems like a micro-optimization, but adding just ACF & Yoast can add 20-30 stylesheets to the editor view which is a lot more rules to be parsing. This would be really difficult to do on a granular basis, but I believe my next point would be able to help with this.
  4. To reduce the number of rules to parse when triggering a change, we could check for valid rules in a stylesheet and only push through those stylesheets which have parsable rules. On the site I'm integrating this into this change + culling out plugins stylesheets cut the stylesheets being passed here from 60+ to 15. This dramatically sped up the rule parsing when triggering the viewport change.

);
}
);
}

function isReplaceableMediaRule( rule ) {
if ( ! rule.media ) {
return false;
}
// Need to use "media.mediaText" instead of "conditionText" for IE support.
return !! rule.media.mediaText.match( VALID_MEDIA_QUERY_REGEX );
}

function replaceRule( styleSheet, newRuleText, index ) {
styleSheet.deleteRule( index );
styleSheet.insertRule( newRuleText, index );
}

function replaceMediaQueryWithWidthEvaluation( ruleText, widthValue ) {
return ruleText.replace( VALID_MEDIA_QUERY_REGEX, ( matchedSubstring ) => {
if (
match( matchedSubstring, {
type: 'screen',
width: widthValue,
} )
) {
return ENABLED_MEDIA_QUERY;
}
return DISABLED_MEDIA_QUERY;
} );
}
Copy link
Member

Choose a reason for hiding this comment

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

This is really cool! 😻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Props to @jorgefilipecosta for most of the replacing logic 😁


/**
* Function that manipulates media queries from stylesheets to simulate a given viewport width.
*
* @param {Array} partialPaths Paths of stylesheets to manipulate.
* @param {number} width Viewport width to simulate.
*/
export default function useSimulatedMediaQuery( width ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we test this properly? unit tests? e2e-tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's a good question.

Unit testing this function is tricky, because the only thing it does is cause DOM-related side-effects. We'd need to mock fetching a stylesheet, build a little "stylesheet" fixture for it to go through, and we'd probably have to reimplement the deleteRule and insertRule methods from CSSStyleSheet (I don't think jsdom has this API). Then we'd need to run it inside a dummy component and check what happens on mount and unmount.

On the other hand, e2e tests would have to rely on specific styles that change across breakpoints. So we'd be checking e.g if randomElement.style.fontSize === '16px' at a certain viewport size, and minor styling changes could easily break them.

All things considered, I'm more inclined towards e2e in this case because they'll test the actual functionality, as opposed to the implementation details.

We could still write unit tests for the smaller functions such as isReplaceableMediaRule and replaceMediaQueryWithWidthEvaluation though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#20159 created to follow up on this.

useEffect( () => {
const styleSheets = getStyleSheetsThatMatchHostname();
Copy link
Contributor

@mikeselander mikeselander Feb 18, 2020

Choose a reason for hiding this comment

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

As far as I can tell, we're pulling stylesheets anew every single time this runs which is unnecessary. I think this could be wrapped in useMemo to prevent it from constantly re-loading.

const originalStyles = [];
styleSheets.forEach( ( styleSheet, styleSheetIndex ) => {
Comment on lines +67 to +69
Copy link
Member

Choose a reason for hiding this comment

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

getStyleSheetsThatMatchHostname can return undefined:

...at which point, forEach will throw an error.

(Aside: TypeScript checking as in #18838 could have caught this)

let relevantSection = false;
for (
let ruleIndex = 0;
ruleIndex < styleSheet.cssRules.length;
++ruleIndex
) {
const rule = styleSheet.cssRules[ ruleIndex ];

if (
! relevantSection &&
!! rule.cssText.match( /#start-resizable-editor-section/ )
) {
relevantSection = true;
}

if (
relevantSection &&
!! rule.cssText.match( /#end-resizable-editor-section/ )
) {
relevantSection = false;
}

if ( ! relevantSection || ! isReplaceableMediaRule( rule ) ) {
continue;
}
const ruleText = rule.cssText;
if ( ! originalStyles[ styleSheetIndex ] ) {
originalStyles[ styleSheetIndex ] = [];
}
originalStyles[ styleSheetIndex ][ ruleIndex ] = ruleText;
replaceRule(
styleSheet,
replaceMediaQueryWithWidthEvaluation( ruleText, width ),
ruleIndex
);
}
} );
return () => {
originalStyles.forEach( ( rulesCollection, styleSheetIndex ) => {
if ( ! rulesCollection ) {
return;
}
for (
let ruleIndex = 0;
ruleIndex < rulesCollection.length;
++ruleIndex
) {
const originalRuleText = rulesCollection[ ruleIndex ];
if ( originalRuleText ) {
replaceRule(
styleSheets[ styleSheetIndex ],
originalRuleText,
ruleIndex
);
}
}
} );
};
}, [ width ] );
}
14 changes: 14 additions & 0 deletions packages/block-editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,20 @@ export function updateSettings( settings ) {
};
}

/**
* Returns an action object used to toggle the width of the editing canvas
*
* @param {string} deviceType
*
* @return {Object} Action object.
*/
export function setPreviewDeviceType( deviceType ) {
return {
type: 'SET_PREVIEW_DEVICE_TYPE',
deviceType,
};
}

/**
* Returns an action object used in signalling that a temporary reusable blocks have been saved
* in order to switch its temporary id with the real id.
Expand Down
19 changes: 18 additions & 1 deletion packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
*/
import { combineReducers } from '@wordpress/data';
import { isReusableBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -1449,6 +1448,23 @@ export function automaticChangeStatus( state, action ) {
// Reset the state by default (for any action not handled).
}

/**
* Reducer returning the editing canvas device type.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function deviceType( state = 'Desktop', action ) {
switch ( action.type ) {
case 'SET_PREVIEW_DEVICE_TYPE':
return action.deviceType;
}

return state;
}

export default combineReducers( {
blocks,
isTyping,
Expand All @@ -1468,4 +1484,5 @@ export default combineReducers( {
lastBlockAttributesChange,
isNavigationMode,
automaticChangeStatus,
deviceType,
} );
11 changes: 11 additions & 0 deletions packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1592,3 +1592,14 @@ export function isNavigationMode( state ) {
export function didAutomaticChange( state ) {
return !! state.automaticChangeStatus;
}

/**
* Returns the current editing canvas device type.
*
* @param {Object} state Global application state.
*
* @return {string} Device type.
*/
export function getDeviceType( state ) {
return state.deviceType;
}
8 changes: 8 additions & 0 deletions packages/block-editor/src/style.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#start-resizable-editor-section {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgefilipecosta Can we use the simulated queries introduced here in the customizer sidebar?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it would work as expected in the customizer view we would useSimulatedMediaQuery with a value that forces a mobile-like view. It will probably not work magically right away and we will probably need some adjustments to make things look well on the customizer like I did in #17960.

display: none;
}

@import "./components/block-icon/style.scss";
@import "./components/block-inspector/style.scss";
@import "./components/block-list/style.scss";
Expand Down Expand Up @@ -38,3 +42,7 @@
@import "./components/warning/style.scss";
@import "./components/writing-flow/style.scss";
@import "./hooks/anchor.scss";

#end-resizable-editor-section {
display: none;
}
8 changes: 8 additions & 0 deletions packages/block-library/src/editor.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#start-resizable-editor-section {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need comments on these?

display: none;
}

@import "./archives/editor.scss";
@import "./audio/editor.scss";
@import "./block/editor.scss";
Expand Down Expand Up @@ -57,3 +61,7 @@
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;
}

#end-resizable-editor-section {
display: none;
}
8 changes: 8 additions & 0 deletions packages/block-library/src/style.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#start-resizable-editor-section {
display: none;
}

@import "./audio/style.scss";
@import "./button/style.scss";
@import "./buttons/style.scss";
Expand Down Expand Up @@ -247,3 +251,7 @@
/*rtl:ignore*/
text-align: right;
}

#end-resizable-editor-section {
display: none;
}
8 changes: 8 additions & 0 deletions packages/block-library/src/theme.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#start-resizable-editor-section {
display: none;
}

@import "./audio/theme.scss";
@import "./code/theme.scss";
@import "./embed/theme.scss";
Expand All @@ -11,3 +15,7 @@
@import "./separator/theme.scss";
@import "./table/theme.scss";
@import "./video/theme.scss";

#end-resizable-editor-section {
display: none;
}
8 changes: 5 additions & 3 deletions packages/e2e-tests/specs/editor/various/preview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ import {
async function openPreviewPage( editorPage ) {
let openTabs = await browser.pages();
const expectedTabsCount = openTabs.length + 1;
await editorPage.click( '.editor-post-preview' );
await editorPage.click( '.editor-post-preview__button-toggle' );
await editorPage.waitFor( '.editor-post-preview__button-external' );
await editorPage.click( '.editor-post-preview__button-external' );

// Wait for the new tab to open.
while ( openTabs.length < expectedTabsCount ) {
Expand All @@ -47,7 +49,7 @@ async function openPreviewPage( editorPage ) {
* @return {Promise} Promise resolving once navigation completes.
*/
async function waitForPreviewNavigation( previewPage ) {
await page.click( '.editor-post-preview' );
await page.click( '.editor-post-preview__button-external' );
return previewPage.waitForNavigation();
}

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

// Disabled until content present.
const isPreviewDisabled = await editorPage.$$eval(
'.editor-post-preview:not( :disabled ):not( [aria-disabled="true"] )',
'.editor-post-preview__button-toggle:not( :disabled ):not( [aria-disabled="true"] )',
( enabledButtons ) => ! enabledButtons.length
);
expect( isPreviewDisabled ).toBe( true );
Expand Down
Loading