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 1 commit
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
Prev Previous commit
Next Next commit
Add indenting with tab
  • Loading branch information
@okmttdhr
okmttdhr committed Mar 27, 2024
commit a3e89a9f6101681a40e49a5a3f4a2758128e50ee
10 changes: 10 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ const restrictedImports = [
message:
'Please use dynamic import (`import()`) instead since it is a large dependency.',
},
{
name: '@codemirror/commands',
message:
'Please use dynamic import (`import()`) instead since it is a large dependency.',
},
{
name: '@codemirror/lang-css',
message:
Expand All @@ -91,6 +96,11 @@ const restrictedImports = [
message:
'Please use dynamic import (`import()`) instead since it is a large dependency.',
},
{
name: '@codemirror/view',
message:
'Please use dynamic import (`import()`) instead since it is a large dependency.',
},
];

module.exports = {
Expand Down
10 changes: 10 additions & 0 deletions package-lock.json
View file
Open in desktop

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

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
"IS_GUTENBERG_PLUGIN": true
},
"dependencies": {
"@codemirror/commands": "6.3.3",
"@codemirror/lang-css": "6.2.1",
"@codemirror/lang-html": "6.4.8",
"@codemirror/view": "6.26.0",
"@wordpress/a11y": "file:packages/a11y",
"@wordpress/annotations": "file:packages/annotations",
"@wordpress/api-fetch": "file:packages/api-fetch",
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,7 +32,9 @@
],
"dependencies": {
"@babel/runtime": "^7.16.0",
"@codemirror/commands": "6.3.3",
"@codemirror/lang-css": "6.2.1",
"@codemirror/view": "6.26.0",
"@emotion/react": "^11.7.1",
"@emotion/styled": "^11.6.0",
"@react-spring/web": "^9.4.5",
Expand Down
8 changes: 5 additions & 3 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
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { Notice, __experimentalVStack as VStack } from '@wordpress/components';
import { useState, useEffect, useRef, useId } from '@wordpress/element';
import { useState, useEffect, useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -101,6 +101,8 @@ export default function AdvancedPanel( {
* @see https://github.com/WordPress/gutenberg/pull/60155
*/
const { EditorView, basicSetup } = await import( 'codemirror' );
const {indentWithTab} = await import('@codemirror/commands');
const {keymap} = await import('@codemirror/view');
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 ) {
Expand All @@ -109,6 +111,7 @@ export default function AdvancedPanel( {
extensions: [
basicSetup,
css(),
keymap.of([indentWithTab]),
EditorView.updateListener.of( ( editor ) => {
if ( editor.docChanged ) {
handleOnChange( editor.state.doc.toString() );
Expand All @@ -130,7 +133,6 @@ export default function AdvancedPanel( {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [] );

const cssEditorId = useId();
return (
<VStack spacing={ 3 }>
{ cssError && (
Expand All @@ -139,7 +141,7 @@ export default function AdvancedPanel( {
</Notice>
) }
<label
htmlFor={ cssEditorId }
htmlFor={ EDITOR_ID }
Copy link
Member

Choose a reason for hiding this comment

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

Apparently a generic div is not labelable. (Confirmed not labeled in the accessibility tree.)

className="block-editor-global-styles-advanced-panel__custom-css-label"
>
{ __( 'Additional CSS' ) }
Expand Down
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,7 +31,9 @@
],
"dependencies": {
"@babel/runtime": "^7.16.0",
"@codemirror/commands": "6.3.3",
"@codemirror/lang-html": "6.4.8",
"@codemirror/view": "6.26.0",
"@wordpress/a11y": "file:../a11y",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/autop": "file:../autop",
Expand Down
3 changes: 3 additions & 0 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 @@ -46,6 +46,8 @@ export default function HTMLEdit( { attributes, setAttributes, isSelected } ) {
* @see https://github.com/WordPress/gutenberg/pull/60155
*/
const { EditorView, basicSetup } = await import( 'codemirror' );
const {indentWithTab} = await import('@codemirror/commands');
const {keymap} = await import('@codemirror/view');
const { html } = await import( '@codemirror/lang-html' );

if ( editorRef.current ) {
Expand All @@ -54,6 +56,7 @@ export default function HTMLEdit( { attributes, setAttributes, isSelected } ) {
extensions: [
basicSetup,
html(),
keymap.of([indentWithTab]),
EditorView.updateListener.of( ( editor ) => {
if ( editor.docChanged ) {
setAttributes( {
Expand Down