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

Add CodeMirror to Additional CSS / Custom HTML block #60155

Open
wants to merge 23 commits into
base: trunk
Choose a base branch
from
+715 −48
Open

Add CodeMirror to Additional CSS / Custom HTML block #60155

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f23e475
Add codemirror to Additional CSS
okmttdhr Mar 25, 2024
7b38263
Add codemirror to Custom HTML block
okmttdhr Mar 25, 2024
b2ceddd
Fix styles
okmttdhr Mar 26, 2024
14678ea
Add focus change effect
okmttdhr Mar 26, 2024
421f2d4
Add codemirror as restrictedImport
okmttdhr Mar 27, 2024
4decdff
Ensure minimum lines to be full-width
okmttdhr Mar 27, 2024
a3e89a9
Add indenting with tab
okmttdhr Mar 27, 2024
9394fc9
Add instructions to focus out of the editor
okmttdhr Mar 27, 2024
15e2486
Align styles with the existing ones
okmttdhr Mar 27, 2024
59a81b5
Fix lint
okmttdhr Mar 27, 2024
4c6262c
Extract EditorView component
okmttdhr Mar 28, 2024
ee88fcd
Fix incorrect use of var()
okmttdhr Mar 28, 2024
a6519f2
Reuse EditorView component
okmttdhr Mar 28, 2024
57f4770
Fix wrong package version
okmttdhr Mar 29, 2024
dff73f6
Fix typo in variable name
okmttdhr Mar 29, 2024
e7aa264
Remove unnecessary condition in useEffect
okmttdhr Apr 1, 2024
fb5ba29
Use @\mixins
okmttdhr Apr 1, 2024
a106416
Add VisuallyHidden component to hide editor instructions
okmttdhr Apr 1, 2024
30ae96e
Fix editor height isn't calculated properly
okmttdhr Apr 1, 2024
af8af11
Use BaseControl for Additional CSS
okmttdhr Apr 1, 2024
b0054fc
Utilize useInstanceId
okmttdhr Apr 2, 2024
ab79885
Switch the editor's light/dark mode
okmttdhr Apr 3, 2024
6a4b77d
Merge branch 'trunk' into add/codemirror
okmttdhr Apr 6, 2024
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
401 changes: 401 additions & 0 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
"IS_GUTENBERG_PLUGIN": true
},
"dependencies": {
"@codemirror/lang-css": "6.2.1",
"@codemirror/lang-html": "6.4.8",
"@wordpress/a11y": "file:packages/a11y",
"@wordpress/annotations": "file:packages/annotations",
"@wordpress/api-fetch": "file:packages/api-fetch",
Expand Down Expand Up @@ -92,6 +94,7 @@
"@wordpress/warning": "file:packages/warning",
"@wordpress/widgets": "file:packages/widgets",
"@wordpress/wordcount": "file:packages/wordcount",
"codemirror": "6.0.1",
"es-module-shims": "^1.8.2",
"wicg-inert": "3.1.2"
},
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
],
"dependencies": {
"@babel/runtime": "^7.16.0",
"@codemirror/lang-css": "6.2.1",
"@emotion/react": "^11.7.1",
"@emotion/styled": "^11.6.0",
"@react-spring/web": "^9.4.5",
Expand Down Expand Up @@ -66,6 +67,7 @@
"@wordpress/wordcount": "file:../wordcount",
"change-case": "^4.1.2",
"classnames": "^2.3.1",
"codemirror": "6.0.1",
"colord": "^2.7.0",
"deepmerge": "^4.3.0",
"diff": "^4.0.2",
Expand Down
87 changes: 67 additions & 20 deletions packages/block-editor/src/components/global-styles/advanced-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
/**
* WordPress dependencies
*/
import {
TextareaControl,
Notice,
__experimentalVStack as VStack,
} from '@wordpress/components';
import { useState } from '@wordpress/element';
import { Notice, __experimentalVStack as VStack } from '@wordpress/components';
import { useState, useEffect, useRef, useId } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -37,40 +33,91 @@ export default function AdvancedPanel( {
}
}
}
function handleOnBlur( event ) {
if ( ! event?.target?.value ) {
function handleOnBlur( newValue ) {
if ( ! newValue ) {
setCSSError( null );
return;
}

const [ transformed ] = transformStyles(
[ { css: event.target.value } ],
[ { css: newValue } ],
'.editor-styles-wrapper'
);

setCSSError(
transformed === null
? __( 'There is an error with your CSS structure.' )
: null
);
}

const editorRef = useRef();
/**
* Ensure the editor has at least min lines of code,
* as the editor will shrink to fit the content.
* @param {string} content The content to ensure min lines for.
* @return {string} The content with at least min lines.
*/
function ensureMinLines( content ) {
const MIN_LINES = 10;
const lines = content.split( '\n' );
const lineCount = lines.length;
let result = content;
for ( let i = lineCount; i < MIN_LINES; i++ ) {
result += '\n';
}
return result;
}
useEffect( () => {
( async () => {
/**
* Lazy load CodeMirror by using Webpack's dynamic import.
* This should be replaced with native dynamic import once it's supported.
* @see https://github.com/WordPress/gutenberg/pull/60155
*/
const { EditorView, basicSetup } = await import( 'codemirror' );
const { css } = await import( '@codemirror/lang-css' );
Copy link
Member

Choose a reason for hiding this comment

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

Importing these individual packages will create four webpack chunks, and the loading will take more time than necessary, because the next load starts only after the previous one has finished.

Better to move the EditorView code into a separate module and load it all at once:

const editorRef = useRef();
useEffect( () => {
  import( './code-mirror-view' ).then( ( { default: createEditor } ) => {
    createEditor( {
      parent: editorRef.current,
      mode: 'css', // or 'html'
      onChange: handleOnChange,
      onBlur: handleOnBlur,
    } );
  } );
}, [] );

Then there will be just one code-mirror-view chunk that's loaded on demand. There's a bit of trouble with the lang-css and lang-html modules. Do we want to bundle them both in a single chunk? That depends on how big they are.

Or the dynamic module can contain the React component that shows the editor: the <div> together with the useRef and the useEffect (this time the effect doesn't do dynamic import). Then it can be used like:

const CodeEditor = React.lazy( () => import( './code-mirror-view' ) );

<Suspense fallback={ <Placeholder /> }>
  <CodeEditor id className aria-describedBy value onChange onBlur />
</Suspense>

It would be also a good idea to not load the CodeMirror editor not when the AdvancedPanel is rendered, that will load it too often. Load it only after clicking on the CSS field, only after the user shows a clear intent to edit.

Copy link
Contributor Author

@okmttdhr okmttdhr Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your review/insights, @jsnajdr! 😄

and the loading will take more time than necessary, because the next load starts only after the previous one has finished

Not exactly, in terms of import. Fundamentally, based on my understanding of how modules are waited for and executed, the following code

import { a } from './a.mjs';
import { b } from './b.mjs';
import { c } from './c.mjs';

console.log(a, b, c);

would be roughly equivalent to the following:

import { promise as aPromise, a } from './a.mjs';
import { promise as bPromise, b } from './b.mjs';
import { promise as cPromise, c } from './c.mjs';

export const promise = Promise.all([aPromise, bPromise, cPromise]).then(() => {
	console.log(a, b, c);
});

At least, Webpack behaves in this manner, even for import(). Here’s a screenshot showing that the requests for chunks run in parallel when using the original approach.

Screenshot 2024-03-28 at 13 47 15

However, this does not seem to be stated explicitly in the spec of import(), and my tests confirmed that native dynamic imports execute requests sequentially both at the top level and within function scopes. I’ll address this later in this comment.

Importing these individual packages will create four webpack chunks

This also applies even if we:

  • consolidate everything into one React component,
  • AND use React.lazy()
  • AND use static import {} from 'codemirror'

because we are splitting the vendor's chunk.

This approach does not create a single chunk, even with all logic extracted into a component and using static import (although we can tweak chunks further by using the optimization.splitChunks option).

Thus, the requests for the “one-component approach” looked like this, showing no difference from the original approach:

Screenshot 2024-03-28 at 13 55 10

Given the above, I propose the following;

  • For explicit parallelization, use Promise.all([import(), import(), …]) rather than fetching a single larger chunk at once. This ensures that requests run in parallel, even after transitioning to native dynamic import.
  • Extract “EditorView”: I favor the one-component approach for its cohesiveness (rather than for performance reasons), so I encapsulated the logic into a component for reuse. 🙂

For now, parallel chunk requests seem to be more efficient.

Then there will be just one code-mirror-view chunk that's loaded on demand. There's a bit of trouble with the lang-css and lang-html modules. Do we want to bundle them both in a single chunk? That depends on how big they are.

I don’t think it’s a good idea to load all lang-* modules in this component because we may want to support additional language modes in the future. (Even with the “parallel requests” strategy, this could potentially max out the browser's limit on simultaneous requests.)

It would be also a good idea to not load the CodeMirror editor not when the AdvancedPanel is rendered, that will load it too often. Load it only after clicking on the CSS field, only after the user shows a clear intent to edit.

On a slow 3G network, this delay could be around 8 seconds. Making users wait that long for syntax highlighting isn't ideal, IMO.

I'd consider more eager loading strategies (if that doesn't interfere with user interaction). For instance, for Additional CSS, we could load the chunks when the three-dots menu is opened, or for a Custom HTML block, load them when the block appears in the search results. Alternatively, prefetch might be a viable approach. WDYT?

Once the chunks are loaded, subsequent network requests are unnecessary, even if users access Additional CSS multiple times. Thus, “loading them too often” shouldn’t pose an issue, in my opinion. We just need to be careful about how it affects users’ actual interactions/experiences.

Copy link
Member

Choose a reason for hiding this comment

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

we are splitting the vendor's chunk

We should consider not doing this. After all, currently Gutenberg doesn't do it on purpose, but only because it's a webpack default and we didn't do any dynamic chunks until now.

In my block lazy loading experiment I disabled it, see this comment: https://github.com/WordPress/gutenberg/pull/55585/files#r1383350711

It helps to keep a nice file structure with human-comprehensible names.


if ( editorRef.current ) {
new EditorView( {
doc: ensureMinLines( customCSS ),
extensions: [
basicSetup,
css(),
EditorView.updateListener.of( ( editor ) => {
if ( editor.docChanged ) {
handleOnChange( editor.state.doc.toString() );
}
} ),
EditorView.focusChangeEffect.of(
( editorState, focusing ) => {
if ( ! focusing ) {
handleOnBlur( editorState.doc.toString() );
}
}
),
],
parent: editorRef.current,
} );
}
} )();
// We only want to run this once, so we can ignore the dependency array.
// eslint-disable-next-line react-hooks/exhaustive-deps
okmttdhr marked this conversation as resolved.
Show resolved Hide resolved
}, [] );

const cssEditorId = useId();
return (
<VStack spacing={ 3 }>
{ cssError && (
<Notice status="error" onRemove={ () => setCSSError( null ) }>
{ cssError }
</Notice>
) }
<TextareaControl
label={ __( 'Additional CSS' ) }
__nextHasNoMarginBottom
value={ customCSS }
onChange={ ( newValue ) => handleOnChange( newValue ) }
onBlur={ handleOnBlur }
className="block-editor-global-styles-advanced-panel__custom-css-input"
spellCheck={ false }
/>
<label
htmlFor={ cssEditorId }
className="block-editor-global-styles-advanced-panel__custom-css-label"
>
{ __( 'Additional CSS' ) }
</label>
<div ref={ editorRef } id={ cssEditorId }></div>
</VStack>
);
}
2 changes: 2 additions & 0 deletions packages/block-library/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
],
"dependencies": {
"@babel/runtime": "^7.16.0",
"@codemirror/lang-html": "6.4.8",
"@wordpress/a11y": "file:../a11y",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/autop": "file:../autop",
Expand Down Expand Up @@ -65,6 +66,7 @@
"@wordpress/wordcount": "file:../wordcount",
"change-case": "^4.1.2",
"classnames": "^2.3.1",
"codemirror": "6.0.1",
okmttdhr marked this conversation as resolved.
Show resolved Hide resolved
"colord": "^2.7.0",
"escape-html": "^1.0.3",
"fast-average-color": "^9.1.1",
Expand Down
49 changes: 37 additions & 12 deletions packages/block-library/src/html/edit.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

For the editor in the Custom HTML block, we'll need to style it in a way that works across different color schemes. This is how it currently looks in a dark color scheme:

Code completion popover has an illegible color combination

Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useContext, useState } from '@wordpress/element';
import {
BlockControls,
PlainText,
useBlockProps,
} from '@wordpress/block-editor';
import { useContext, useState, useRef, useEffect } from '@wordpress/element';
import { BlockControls, useBlockProps } from '@wordpress/block-editor';
import {
ToolbarButton,
Disabled,
Expand All @@ -27,6 +23,8 @@ export default function HTMLEdit( { attributes, setAttributes, isSelected } ) {

const instanceId = useInstanceId( HTMLEdit, 'html-edit-desc' );

const editorRef = useRef();

function switchToPreview() {
setIsPreview( true );
}
Expand All @@ -40,6 +38,38 @@ export default function HTMLEdit( { attributes, setAttributes, isSelected } ) {
'aria-describedby': isPreview ? instanceId : undefined,
} );

useEffect( () => {
( async () => {
/**
* Lazy load CodeMirror by using Webpack's dynamic import.
* This should be replaced with native dynamic import once it's supported.
* @see https://github.com/WordPress/gutenberg/pull/60155
*/
const { EditorView, basicSetup } = await import( 'codemirror' );
const { html } = await import( '@codemirror/lang-html' );

if ( editorRef.current ) {
new EditorView( {
doc: attributes.content,
extensions: [
basicSetup,
html(),
EditorView.updateListener.of( ( editor ) => {
if ( editor.docChanged ) {
setAttributes( {
content: editor.state.doc.toString(),
} );
}
} ),
],
parent: editorRef.current,
} );
}
} )();
// Run this only when the UI renders, so we can ignore the dependency array.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ isPreview, isDisabled ] );

return (
<div { ...blockProps }>
<BlockControls>
Expand Down Expand Up @@ -73,12 +103,7 @@ export default function HTMLEdit( { attributes, setAttributes, isSelected } ) {
</VisuallyHidden>
</>
) : (
<PlainText
value={ attributes.content }
onChange={ ( content ) => setAttributes( { content } ) }
placeholder={ __( 'Write HTML…' ) }
aria-label={ __( 'HTML' ) }
/>
<div ref={ editorRef } aria-label={ __( 'HTML' ) } />
) }
</div>
);
Expand Down
24 changes: 5 additions & 19 deletions packages/edit-site/src/components/global-styles/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,25 +119,11 @@
margin: $grid-unit-20;

.components-v-stack {
flex: 1 1 auto;

.block-editor-global-styles-advanced-panel__custom-css-input {
flex: 1 1 auto;
display: flex;
flex-direction: column;

.components-base-control__field {
flex: 1 1 auto;
display: flex;
flex-direction: column;

.components-textarea-control__input {
flex: 1 1 auto;
// CSS input is always LTR regardless of language.
/*rtl:ignore*/
direction: ltr;
}
}
.block-editor-global-styles-advanced-panel__custom-css-label {
font-size: 11px;
font-weight: 500;
line-height: 1.4;
text-transform: uppercase;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tools/webpack/packages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ module.exports = {
}
return `webpack://${ info.namespace }/${ info.resourcePath }`;
},
chunkFilename: './build/[name]/[name]-[contenthash].min.js',
},
performance: {
hints: false, // disable warnings about package sizes
Expand Down
Loading