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

Add CodeMirror to the HTML block #4348

Merged
merged 12 commits into from
Feb 27, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jan 8, 2018

👀 What this is

Adds CodeMirror to the HTML block. This means that a user can type their custom HTML using a fully featured text editor with linting and syntax highlighting:

codemirror

🤔 How this works

CodeMirror was added to core in WordPress 4.9 which means we can enhance a <textarea> simply by calling wp.codeEditor.initialize().

To make CodeMirror easier to use within React, I introduced a <CodeEditor> component which acts as a drop-in replacement for a <textarea>.

I've also incorporated some of @westonruter's work in #1625 by using SandBox within our HTML block. This makes previewing a HTML block more robust. Specifically, you can now insert HTML that blocks rendering, e.g.:

<script>document.write( 'Hey!' );</script>

To avoiding adding a whopping 1.9 MB to our assets, I've implemented a very simple mechanism to lazily load the assets that are normally queued by calling wp_enqueue_code_editor().

✅ How to test

  1. Create a HTML block
  2. Preview it
  3. Save it, view the page
  4. Check that you can preview HTML that blocks rendering, e.g. <script>document.write( 'Hey!' );</script>
  5. Log in as an author and test that you get lint warnings when using e.g. the <script> tag

@noisysocks noisysocks added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Jan 8, 2018
@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch 2 times, most recently from 2eeb4c5 to 37445c7 Compare January 8, 2018 06:24
@gziolo
Copy link
Member

gziolo commented Jan 8, 2018

Cross-posting related issue: #2768. I didn't take any measurements on the size of CodeMirror, but I would expect it isn't a small one :) I think it's fine to include it temporarily on the initial editor load. However we should at least double check what impact it has on the size of JS and CSS loaded, and mention it in the #2768 so we know if we have to act before it gets merged into the core.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 8, 2018

Thanks @gziolo. Some quick measurements from running npm run build and checking DevTools:

Branch All JS CSS
master 3.4 MB 2.9 MB 261 KB
This PR 5.3 MB 4.8 MB 277 KB

A 1.9 MB (55%) increase! 😱

@noisysocks
Copy link
Member Author

noisysocks commented Jan 8, 2018

cc'ing some people who might want to track this work:

@gziolo
Copy link
Member

gziolo commented Jan 9, 2018

A 1.9 MB (55%) increase! 😱

@noisysocks thanks for checking 🙇

Another thing we can add as a follow-up PR is HTML linting which is built-in in the core. By the way, the code for linting is lazy-loaded when editing plugin files. Which is a very good sign since we discovered that CodeMirror adds 1.9 MB to the uncompressed build size.

} ) );

const wrapper = shallow( <CodeEditor value={ '<b>wowee</b>' } /> );
expect( wrapper.find( 'textarea' ) ).toHaveLength( 1 );
Copy link
Member

Choose a reason for hiding this comment

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

We could use snapshot testing here, too. See example:

expect( wrapper ).toMatchSnapshot();
and docs: https://github.com/WordPress/gutenberg/blob/master/docs/testing-overview.md#snapshot-testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I've changed this to a snapshot test in 17dbeb231a901007b08e5fd0dd75840bdbbc1489.

@noisysocks noisysocks changed the title [WIP] Add syntax highlighting to the Custom HTML block Add CodeMirror to the HTML block Jan 9, 2018
@noisysocks noisysocks removed the [Status] In Progress Tracking issues with work in progress label Jan 9, 2018
@mtias
Copy link
Member

mtias commented Jan 9, 2018

A 1.9 MB (55%) increase! 😱

Ouch.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from 17dbeb2 to e2c619d Compare January 10, 2018 02:28
@noisysocks noisysocks added the [Type] Enhancement A suggestion for improvement. label Jan 11, 2018
@noisysocks
Copy link
Member Author

noisysocks commented Jan 16, 2018

Any feedback, questions, or concerns with this PR?

I'd like to get the basic functionality merged and then iterate on it. I've started work on lazily loading the assets over in #4500.

@jasmussen
Copy link
Contributor

jasmussen commented Jan 16, 2018

Really nice work, fun to see this, it's a very very cool block.

There are a couple of things we should address in this PR. First off, can we tweak the padding a little bit so it looks like this?

screen shot 2018-01-16 at 13 36 08

I think this should do it:

.CodeMirror-lines {
padding: 8px 8px 8px 0;
}

There's a z-index issue with adjecent block toolbars:

screen shot 2018-01-16 at 13 33 12

One important issue we have to fix is that you need to be able to escape this block using just the keyboard:

  • Pressing Escape while you're editing inside the block should deselect the entire block, just like it does in a Paragraph block
  • Pressing the ↓ at the end of a codemirror block should move the caret into the next adjecent block, same with ↑ at the beginning of a block
  • Ideally you'd also be able to press Enter twice at the end of a codemirror block to create a new Paragraph block below, just like how you can press Enter twice inside a list to exit it

Question for a future PR, doesn't have to be this one — can we customize the colors of the syntax highlighting? If yes, I'd love for us to use more WordPressy colors. Alternatively, I'd love it if we could at least use something like $blue-medium-200 for the line-highlighter.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 17, 2018

Question for a future PR, doesn't have to be this one — can we customize the colors of the syntax highlighting?

Yep! It's just a matter of styling the .CodeMirror classes 🙂

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from e2c619d to 7714d1e Compare January 18, 2018 03:53
@noisysocks
Copy link
Member Author

Thanks @jasmussen! I've implemented your suggested changes.

Pressing the ↓ at the end of a codemirror block should move the caret into the next adjecent block, same with ↑ at the beginning of a block

I made it so that pressing UP/DOWN first moves the cursor to the start/end of the line. A subsequent press will then move focus to the previous/next block. It feels really natural in my testing.

Ideally you'd also be able to press Enter twice at the end of a codemirror block to create a new Paragraph block below, just like how you can press Enter twice inside a list to exit it

I didn't implement this since pressing ENTER twice is quite common while writing code and e.g. adding whitespace between lines.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from 7714d1e to 7847ea2 Compare January 18, 2018 04:14
@dmsnell
Copy link
Member

dmsnell commented Jan 18, 2018

sorry I'm coming in somewhat late. what did we decide about async-loading CodeMirror? are we wanting to still do that?

we can use example code in #665 where I have everything async-loading including the editor and the language modes

I don't think it's responsible for us to add almost 2 MB to the download, especially since we expect most page views to not open the HTML editor. I'd be happy to help with this too, though my cycles lately have been limited and stretched

@westonruter
Copy link
Member

sorry I'm coming in somewhat late. what did we decide about async-loading CodeMirror? are we wanting to still do that?

Yes, see #4500

@noisysocks
Copy link
Member Author

Any more thoughts on this? 😊

@dmsnell
Copy link
Member

dmsnell commented Jan 22, 2018

I'd really like to see us wait until this is loading on-demand before merging it otherwise it's going to instantly bloat the editor in no small way - that is, I'd love to see something like #4500 merge first or have some changes here to lazy-load.

@noisysocks
Copy link
Member Author

I'll hold off on hitting the merge button until #4500 is ready. Nonetheless, I'd like to get this PR into an approved and ready-to-go state 🙂

@jasmussen
Copy link
Contributor

Visually this is in a good place 👍:

screen shot 2018-01-23 at 09 05 26

I also appreciate that the arrow at the beginning and end of the block enters adjecent blocks or creates a new paragraph so at least you can escape the block. 👍

However I'm seeing some weird behavior when testing. Not sure if it's intentional, but basic dummy text paragraphs are joined together on reload:

codeblock

@noisysocks
Copy link
Member Author

noisysocks commented Jan 23, 2018

However I'm seeing some weird behavior when testing. Not sure if it's intentional, but basic dummy text paragraphs are joined together on reload:

It looks like this is because Gutenberg pretty prints the post HTML before saving. You can see that this happens on master as well. I think this behaviour is reasonable. We're typing HTML into the block here and, in HTML, whitespace is collapsed.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from d9cbfaa to e51f342 Compare January 29, 2018 02:29
@noisysocks
Copy link
Member Author

noisysocks commented Jan 29, 2018

As per the discussion over in #4500, I've implemented lazy loading in a one-off manner in e51f342. The 1.9 MB of CodeMirror scripts and styles are now not loaded until a HTML block is instantiated.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch 4 times, most recently from 5c729cc to 35d7dde Compare February 8, 2018 00:21
@noisysocks
Copy link
Member Author

Rebased and fixed conflicts. How do we feel about getting this in? cc. @westonruter @mtias

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

A couple minor things I commented on.

Also, there is a bit of an unexpected UX behavior whereby if I shift-arrow to select some text, I get the block toolbar appearing. I understand that normally makes sense because toolbar buttons like bold and italic. But since there are no such buttons in the toolbar for Custom HTML (yet, as I suppose there could be shortcuts to add common tags), it is a little distracting to see the HTML/Preview buttons flash whenever I select some text with the keyboard.

return (
<CodeEditor
value={ '<p>This is some <b>HTML</b> code that will have syntax highlighting!</p>' }
onChange={ value => console.log( value ) }
Copy link
Member

Choose a reason for hiding this comment

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

Attributes here have inconsistent indentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Fixed in 87af208.

=======

CodeEditor is a React component that provides the user with a code editor
that has syntax highliting and linting.
Copy link
Member

Choose a reason for hiding this comment

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

s/highliting/highlighting/

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Fixed in 87af208.

this.lastCursor = editor.getCursor();
}

onKeyHandled( editor, name, event ) {
Copy link
Member

Choose a reason for hiding this comment

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

I love this.

@westonruter
Copy link
Member

Per my review comment, here's me with cursor in block:

image

And then as soon as I select some text with keyboard:

image

It doesn't feel like the HTML/Preview buttons show appear.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from 35d7dde to 87af208 Compare February 26, 2018 03:16
Introduces a new <CodeEditor> component which wraps a CodeMirror
enhanced <textarea>. This component is then used to provide syntax
highliting in the Custom HTML block.
This prevents the cursor from moving around to different blocks while
editing code.
Make the CodeEditor in the HTML block look vaguely like what Joen mocked
up in #1386.
Prevent the iframe from capturing pointer events and preventing the user
from being able to select the block.
`componentDidUpdate` and `componentDidMount` fire after DOM
reconcilation but before the frame is painted. CodeMirror's `focus()`
method seems to only work *after* paint. Scheduling `focus()` to happen
after the next frame is painted seems to be the best that we can do.
We only care that the component renders without an error and doesn't
change unintentionally, which makes this a perfect use case for snapshot
testing.
Bumps up the z-index of every element that is at the same depth as the
block toolbar. By making these indicies greater than 4, we ensure our
contextual elements appear over the CodeMirror editor. This stops the
CodeMirror gutter from appearing over the block toolbar.
- Permit pressing ESCAPE to unfocus the currently selected block.
- Allow pressing UP/DOWN to move focus to the previous/next block when
  the cursor is at the start/end of the editor.
Wraps CodeEditor with a component that lazily loads the scripts and
styles that CodeMirror needs. This lets us avoid using
wp_enqueue_code_editor() which adds considerable bulk (~ 1.9 MB) to the
page load.
@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from 87af208 to 30ade66 Compare February 27, 2018 01:16
@noisysocks
Copy link
Member Author

noisysocks commented Feb 27, 2018

Thanks for reviewing this, @westonruter!

It doesn't feel like the HTML/Preview buttons show appear.

Good catch. I agree that it feels weird. I think to address this we need to split <Slot name="Block.Toolbar"> into two slots: one for toolbar buttons that affect the block and one for toolbar buttons that affect the selected text. Only the latter should be shown when text is selected.

Since this would require moving a few things around and it's not a major issue, I'll defer this work for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants