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

Move the HTML block into the blocks library package #10396

Merged
merged 3 commits into from
Oct 9, 2018

Conversation

youknowriad
Copy link
Contributor

In order to merge Gutenberg into Core easily, all the Gutenberg JavaScript Core must be shared a reusable npm package. This PR updates the HTML block to avoid relying on wp.codeEditor for now because there's no easy way to lazy-load WordPress scrips yet and move the HTML block into the blocks library.

The wp.components.CodeEditor has also been deprecated with this change.

Testing instructions

  • Make sure you can use HTML block properly.
  • There's no syntax highlighting for the moment but it's consistent with the Code Block and could be added later in a better way.

@youknowriad youknowriad added the npm Packages Related to npm packages label Oct 8, 2018
@youknowriad youknowriad self-assigned this Oct 8, 2018
@youknowriad youknowriad requested review from mcsf, gziolo and a team October 8, 2018 07:53
@youknowriad youknowriad force-pushed the update/move-html-blokc-to-packages branch from 924b0e5 to febc7dc Compare October 8, 2018 08:07
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks good from my perspective.

Should we get design feedback for this one as it changes the UX for the Code block?

Should we merge #10397 in this PR and test them together again? I think we might want to add Code block inside the changelog, the way we did it for Classic block in #10397.


deprecated( 'wp.components.CodeEditor', {
version: '4.2',
alternative: 'wp.codeEditor directly or any alternative React component for syntax highlighting',
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 going to be confusing in the context of npm package which is out of WordPress context. Not sure if this is bad or not, just noting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we have a bunch of those and we need to experiment with making the messages opt-in or something and only enable in WP

@gziolo gziolo added this to the 4.0 milestone Oct 8, 2018
@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Oct 8, 2018
@chrisvanpatten
Copy link
Contributor

Sad to lose syntax highlighting but it looks good to me, especially since it mirrors the code block. Hopefully in Phase 2 it'll be possible to get a better code editing solution in place across the board.

Only thing to wonder about is whether custom HTML and code will get confused for each other but honestly I think we have that problem either way.

👍 from me!

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Oct 9, 2018

I'm worried that people are going to complain a lot about the loss of functionality. It is easy to explain to a developer why the syntax highlighting may have to be removed for now, but it is far harder to explain to the end user. To some, I am sure this will seem like a sign that Gutenberg is cutting corners to try and meet the deadline.

Personally, I really like the syntax highlighting and would not like to see it be removed at all. If anything, I wish the standard Edit as HTML option and Code Editor mode used syntax highlighting.

@youknowriad
Copy link
Contributor Author

Agreed, it would be great to have syntax Highlighting but personally, I do think the current syntax highlighting is not great anyway and should be improved. We should explore alternatives to CodeMirror. (Monaco Editor is a very good option these days).

Removing this from the npm package (where shipping Lazy Loading in a generic way is not really possible) doesn't mean we can't intergrate it back from Core Side but I do think we gain clarity by doing so as the HTML block is something any Gutenberg install (CMS agnostic) could benefit from.

@jasmussen
Copy link
Contributor

jasmussen commented Oct 9, 2018

Thanks for working on this.

This is an interesting crossroads to face. The syntax highlighter is delicious and wonderful, but at the same time if we can't lazy-load it now, it comes with a performance hit.

Difficult. But in order to not stall momentum, I think we should do this:

  • Create a future-milestoned ticket to add back the syntax highlighter when we can lazy load it
  • Ship this in the meantime.

To some, I am sure this will seem like a sign that Gutenberg is cutting corners to try and meet the deadline.

This is the difficulty of making software. On one hand there's a great desire to have "everything ready" for the first version. On the other hand, the longer we wait to ship, the longer we deny ourselves the much needed testing surface that is the entire web. Although Gutenberg at the time of writing is on 530.000 sites, there are still many rough corners we need to sand off before shipping, which means we need to make tough decisions related to focus. In that vein, the most important thing is to ship a 1.0.

On the flipside, if we all pull together and ship the 1.0, then the 1.1 can hopefully be released as early as March 2019.

@youknowriad
Copy link
Contributor Author

I created this issue to track it #10423

* Move the Classic block to the packages

* Rename freeform => classic

* Update the changelog of the block library package

* Fix classic block editor styles
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 Needs Design Feedback Needs general design feedback. npm Packages Related to npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants