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.

Sign up for GitHub

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

Jump to bottom

Apply the editor styles to the Site Editor page #20982

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Apply the editor styles to the Site Editor page #20982

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
254e418
Apply the editor styles to the Site Editor page
youknowriad Mar 18, 2020
6d4ff60
Fix tests
youknowriad Mar 25, 2020
82c296c
Fix mobile tests
youknowriad Mar 26, 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
50 changes: 50 additions & 0 deletions lib/edit-site-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,55 @@ function gutenberg_is_edit_site_page( $page ) {
return in_array( $page, $allowed_pages, true );
}

/**
* Load editor styles (this is copied form edit-form-blocks.php).
* Ideally the code is extracted into a reusable function.
*
* @return array Editor Styles Setting.
*/
function gutenberg_get_editor_styles() {
global $editor_styles;

//
// Ideally the code is extracted into a reusable function.
$styles = array(
array(
'css' => file_get_contents(
ABSPATH . WPINC . '/css/dist/editor/editor-styles.css'
),
),
);

/* translators: Use this to specify the CSS font family for the default font. */
$locale_font_family = esc_html_x( 'Noto Serif', 'CSS Font Family for Editor Font' );
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
$styles[] = array(
'css' => "body { font-family: '$locale_font_family' }",
);

if ( $editor_styles && current_theme_supports( 'editor-styles' ) ) {
foreach ( $editor_styles as $style ) {
if ( preg_match( '~^(https?:)?//~', $style ) ) {
$response = wp_remote_get( $style );
if ( ! is_wp_error( $response ) ) {
$styles[] = array(
'css' => wp_remote_retrieve_body( $response ),
);
}
} else {
$file = get_theme_file_path( $style );
if ( is_file( $file ) ) {
$styles[] = array(
'css' => file_get_contents( $file ),
'baseURL' => get_theme_file_uri( $style ),
);
}
}
}
}

return $styles;
}

/**
* Initialize the Gutenberg Edit Site Page.
*
Expand Down Expand Up @@ -152,6 +201,7 @@ function gutenberg_edit_site_init( $hook ) {
$settings['templateType'] = 'wp_template';
$settings['templateIds'] = array_values( $template_ids );
$settings['templatePartIds'] = array_values( $template_part_ids );
$settings['styles'] = gutenberg_get_editor_styles();

// This is so other parts of the code can hook their own settings.
// Example: Global Styles.
Expand Down
38 changes: 38 additions & 0 deletions packages/block-editor/src/components/editor-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* External dependencies
*/
import { compact, map } from 'lodash';

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

/**
* Internal dependencies
*/
import transformStyles from '../../utils/transform-styles';

function EditorStyles( { styles } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Was there any consideration to make this a hook, considering its own behaviors are hooks? And observing that the current implementation forced refactoring of EditorProvider to render a fragment to support the additional top-level component.

Copy link
Contributor Author

@youknowriad youknowriad Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about it, the main reason I didn't do it is because block-editor don't expose hooks but expose similar behavior components like "ObserveTyping", "WritingFlow". I guess even this behaviors could be hooks.

Now, that I think about it more, maybe a hook is better though.

Copy link
Member

@aduth aduth Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I was thinking about these "behavioral" components as well. They made sense at the time. Nowadays, I feel that hooks are a perfect fit for how and why we were implementing those.

There's still an argument that could be made of "consistency for consistency's sake", so I could be okay with either approach.

useEffect( () => {
const updatedStyles = transformStyles(
styles,
'.editor-styles-wrapper'
);

const nodes = map( compact( updatedStyles ), ( updatedCSS ) => {
const node = document.createElement( 'style' );
node.innerHTML = updatedCSS;
document.body.appendChild( node );

return node;
} );

return () =>
nodes.forEach( ( node ) => document.body.removeChild( node ) );
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Related to #20982 (comment), I observe this unmounting behavior is "new", though again not entirely clear what impact (if any) it would have.

One worry I might have is that since this unsubscribe behavior is called if any dependencies change, are we sure that styles will remain stable? Aside from my current debugging of test failures, seems like it might be a performance concern if the references change with any frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we sure that styles will remain stable

I think so since these come from the server. It would surprise if this is the reason for the failure.

The reason i added the unmount behavior is because it's the right thing to do if the styles changes, we need to recompute and reload them.

}, [ styles ] );

return null;
}

export default EditorStyles;
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export { default as BlockTitle } from './block-title';
export { default as BlockToolbar } from './block-toolbar';
export { default as CopyHandler } from './copy-handler';
export { default as DefaultBlockAppender } from './default-block-appender';
export { default as __unstableEditorStyles } from './editor-styles';
export { default as Inserter } from './inserter';
export { default as BlockEditorKeyboardShortcuts } from './keyboard-shortcuts';
export { default as MultiSelectScrollIntoView } from './multi-select-scroll-into-view';
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { EntityProvider } from '@wordpress/core-data';
import {
__experimentalEditorSkeleton as EditorSkeleton,
__experimentalFullscreenMode as FullscreenMode,
__unstableEditorStyles as EditorStyles,
} from '@wordpress/block-editor';
import { useViewportMatch } from '@wordpress/compose';

Expand Down Expand Up @@ -61,6 +62,7 @@ function Editor( { settings: _settings } ) {

return template ? (
<>
<EditorStyles styles={ settings.styles } />
<FullscreenMode isActive={ isFullscreenActive } />
<SlotFillProvider>
<DropZoneProvider>
Expand Down
63 changes: 26 additions & 37 deletions packages/editor/src/components/provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import { Component } from '@wordpress/element';
import { withDispatch, withSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { EntityProvider } from '@wordpress/core-data';
import { BlockEditorProvider, transformStyles } from '@wordpress/block-editor';
import {
BlockEditorProvider,
__unstableEditorStyles as EditorStyles,
} from '@wordpress/block-editor';
import apiFetch from '@wordpress/api-fetch';
import { addQueryArgs } from '@wordpress/url';
import { decodeEntities } from '@wordpress/html-entities';
Expand Down Expand Up @@ -134,23 +137,6 @@ class EditorProvider extends Component {

componentDidMount() {
this.props.updateEditorSettings( this.props.settings );

if ( ! this.props.settings.styles ) {
return;
}
Comment on lines -138 to -140
Copy link
Member

Choose a reason for hiding this comment

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

Where did this condition go in the new implementation? Was it not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed cause we map through the styles but it can be micro-optim


const updatedStyles = transformStyles(
this.props.settings.styles,
'.editor-styles-wrapper'
);

map( updatedStyles, ( updatedCSS ) => {
if ( updatedCSS ) {
const node = document.createElement( 'style' );
node.innerHTML = updatedCSS;
document.body.appendChild( node );
}
} );
}

componentDidUpdate( prevProps ) {
Expand Down Expand Up @@ -197,27 +183,30 @@ class EditorProvider extends Component {
);

return (
<EntityProvider kind="root" type="site">
<EntityProvider
kind="postType"
type={ post.type }
id={ post.id }
>
<BlockEditorProvider
value={ blocks }
onInput={ resetEditorBlocksWithoutUndoLevel }
onChange={ resetEditorBlocks }
selectionStart={ selectionStart }
selectionEnd={ selectionEnd }
settings={ editorSettings }
useSubRegistry={ false }
<>
<EditorStyles styles={ settings.styles } />
<EntityProvider kind="root" type="site">
<EntityProvider
kind="postType"
type={ post.type }
id={ post.id }
>
{ children }
<ReusableBlocksButtons />
<ConvertToGroupButtons />
</BlockEditorProvider>
<BlockEditorProvider
value={ blocks }
onInput={ resetEditorBlocksWithoutUndoLevel }
onChange={ resetEditorBlocks }
selectionStart={ selectionStart }
selectionEnd={ selectionEnd }
settings={ editorSettings }
useSubRegistry={ false }
>
{ children }
<ReusableBlocksButtons />
<ConvertToGroupButtons />
</BlockEditorProvider>
</EntityProvider>
</EntityProvider>
</EntityProvider>
</>
);
}
}
Expand Down