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

Editor: HTML Editor per block #2797

Merged
merged 12 commits into from
Oct 10, 2017
Merged

Editor: HTML Editor per block #2797

merged 12 commits into from
Oct 10, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Sep 26, 2017

Description

This PR explores the idea in #2794 This is largely unfinished yet but this shows how this could work. The button to switch to HTML mode is located on the menu on the right of block instead of the inspector because it's not possible to keep showing the inspector controls while in HTML mode (this is probably temporary, we need some design input here)

Checklist:

  • Button to toggle the mode per block
  • BlockHTML form
  • Run the block validation when switching back to the visual mode
  • Make this pretty
  • Add unit tests

Screenshots (jpeg or gifs if applicable):

screen shot 2017-09-26 at 14 47 27

@mtias
Copy link
Member

mtias commented Sep 26, 2017

There's a little "HTML Code" icon we use for the HTML block that should be used if we need an icon.

@youknowriad
Copy link
Contributor Author

@mtias Actually, I'm already using it when you're still on the visual mode, but the icon changes when you trigger the HTML mode. Probably a bad choice but you know, early iterations :)

@mcsf
Copy link
Contributor

mcsf commented Sep 26, 2017

It would be cool if, when switching back to Visual, the same kind of prompts would be seen as when returning from global HTML to global Visual mode:

screen shot 2017-09-26 at 15 19 19

(For the record, currently, the changes performed in per-block HTML mode are discarded and the visual block is rendered as it previously was.)

@youknowriad
Copy link
Contributor Author

@mcsf I was thinking it was already the case, but I think you're right, I need to run the validation manually

@mtias
Copy link
Member

mtias commented Sep 26, 2017

There's a case to be made that opting into the HTML mode converts it to an HTML block entirely.

@mtias
Copy link
Member

mtias commented Sep 26, 2017

Also, somewhat related, the invalid dialog should include an option to edit as an HTML block, not just Classic Text.

@jasmussen
Copy link
Contributor

Love this feature!

Designwise I think we'll want to think about this for a bit. I'm concerned with adding still more buttons to the side. (Oh and tiny nitpick on current implementation, the hover text says "Delete the block").

For a while I've been suggesting that we implement an overflow menu — that is, an ellipsis that sits at the end of every toolbar. On click it shows a dropdown with items that don't fit in the menu. This is especially important for mobile, where the toolbar might not fit. This space would be perfect for an "Edit HTML" button, I think. What do you think, @karmatosed

@youknowriad
Copy link
Contributor Author

@jasmussen Agreed about the design issues, If we can have a mockup or I'd be to try to implement this (maybe separately)

@aduth
Copy link
Member

aduth commented Sep 27, 2017

Random thought: With these changes, could we potentially consider dropping the Visual / Text toggle altogether? Maybe a bit too controversial 😉

Edit: Aha, late to the party as always, I see some discussion of this impact in #2794 already.

@youknowriad
Copy link
Contributor Author

youknowriad commented Sep 27, 2017

Random thought: With these changes, could we potentially consider dropping the Visual / Text toggle altogether? Maybe a bit too controversial

I'm also in favour of this :)

@karmatosed
Copy link
Member

Random thought: With these changes, could we potentially consider dropping the Visual / Text toggle altogether? Maybe a bit too controversial

I would suggest usability testing on that. I am not personally against it though, it always has been confusing.

@jasmussen
Copy link
Contributor

Let's step back from the cliff for a second and regroup.

There's a use case for people who never want to use the visual editor, they just want the text editor. We should let them — they might never use Gutenblocks at all, and would have no HTML comments. We should even cookie their choice, so if they are in HTML mode and reload the page, they stay there.

For the occasions where you want to fix tag soup, it makes total sense to be able to edit on a per block basis. Having a "edit HTML" button in context of the block itself is also a delightful callback to the Visual/HTML tabs of the current editor.

As such it seems like there are some opportunities here:

a) rethinking where the editor mode switch lives. Perhaps in the ellipsis menu as suggested in another thread

b) if we can add an unread notification dot to the ellipsis menu, perhaps even a pointer bubble when new plugins add themselves there (see also #2460 ), then when a page builder installs itself, it can draw attention to the "mode switch" there

If we think this is cool, the big challenge then becomes: where do we put this html mode switch, without overloading the toolbar? Is this switch discoverable enough if we were to add an ellipsis dropdown at the end of every toolbar?

@youknowriad
Copy link
Contributor Author

One crazy idea I was thinking of for the Text mode is using Gutenberg (or precisely a block list) also for the text mode. Let me detail this:

  • Let's say we split Gutenberg into reusable components, one of these components is the BlockList component where we provide an array of blocks to display and also a list of available blocks to add.

  • We use a BlockList with only a unique block type: HTML block, which means the Text mode is a list of HTML blocks, the comments stay hidden, the user can't mess with them.

  • The "serializer" for this Text mode is not the regular Gutenberg serializer, but just a simple one which concats the previous comments with the new HTML content.

@jasmussen
Copy link
Contributor

I utterly love that idea, Riad.

@ellatrix
Copy link
Member

@youknowriad @jasmussen I proposed this a while ago: https://wordpress.slack.com/archives/C02QB2JS7/p1496215809472978

One advantage there too is that we can scroll to the same block the switching mode.

@jasmussen
Copy link
Contributor

jasmussen commented Sep 29, 2017

Here are mockups of what it would look like if the HTML mode was in the quick toolbar ellipsis menu:

edit html

edit html ellipsis toggled

I keep coming back to the quick toolbar having an ellipsis dropdown being an important task we should start work on soon. We can also choose to show as many items in the quick toolbar as there's space for, and then only "pop off" buttons to the ellipsis menu when we run out of space. In this case there would be space for the two new buttons, so it would look like this:

edit html wide quick toolbar

Here's where the global document switch would live:

edit html global switch

Note, the "footnote" and "spell checking" items are simply examples of what other elements could live in those overflow menus.

Edit: another HTML switcher mockup:

choose editor

CC: @mtias @karmatosed @youknowriad

@youknowriad youknowriad force-pushed the try/html-per-block branch 2 times, most recently from 3e2458b to 3d65fd6 Compare September 29, 2017 10:15
@codecov
Copy link

codecov bot commented Sep 29, 2017

Codecov Report

Merging #2797 into master will decrease coverage by 0.06%.
The diff coverage is 30.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2797      +/-   ##
==========================================
- Coverage   33.92%   33.86%   -0.07%     
==========================================
  Files         193      194       +1     
  Lines        5701     5741      +40     
  Branches     1000     1009       +9     
==========================================
+ Hits         1934     1944      +10     
- Misses       3187     3212      +25     
- Partials      580      585       +5
Impacted Files Coverage Δ
editor/actions.js 36.84% <0%> (-2.05%) ⬇️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-html.js 0% <0%> (ø)
editor/block-settings-menu/content.js 0% <0%> (ø) ⬆️
editor/selectors.js 96.77% <100%> (+0.02%) ⬆️
blocks/api/serializer.js 98.11% <100%> (+0.11%) ⬆️
editor/reducer.js 88.81% <85.71%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c80ac15...0a5ae79. Read the comment docs.

@aduth
Copy link
Member

aduth commented Sep 29, 2017

There's a use case for people who never want to use the visual editor, they just want the text editor. We should let them — they might never use Gutenblocks at all, and would have no HTML comments. We should even cookie their choice, so if they are in HTML mode and reload the page, they stay there.

Couldn't these users just insert a single HTML block into the editor and have-at-it?

aduth
aduth previously requested changes Sep 29, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Strange behavior I'm seeing:

  1. Navigate to Gutenberg > Demo
  2. Switch the first Cover Image block to HTML mode
  3. Focus in the block
  4. Focus out of the block

The block changes its HTML inexplicably, resetting its content to the default state.

className="editor-block-settings-menu__control"
onClick={ onToggleMode }
icon="html"
label={ __( 'Switch between the visual/textual mode' ) }
Copy link
Member

Choose a reason for hiding this comment

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

TIL "textual" is a word - still think "HTML" or "text" might be clearer.

Copy link
Member

Choose a reason for hiding this comment

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

TIL "textual" is a word - still think "HTML" or "text" might be clearer.

Still think this ought to be addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry forgot about this, updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going for "text" for consistency with the global mode switcher

@@ -138,6 +138,20 @@ export const editor = combineUndoableReducers( {
},
};

case 'UPDATE_BLOCK':
// Ignore updates if block isn't known
Copy link
Member

Choose a reason for hiding this comment

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

Tab one more.

componentWillReceiveProps( nextProps ) {
if ( ! isEqual( nextProps.block.attributes, this.state.attributes ) ) {
this.setState( {
html: getBlockContent( nextProps.block ),
Copy link
Member

Choose a reason for hiding this comment

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

Would we need to update attributes state here as well?

this.onBlur = this.onBlur.bind( this );
this.state = {
html: props.block.isValid ? getBlockContent( props.block ) : props.block.originalContent,
attributes: props.block.attributes,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to maintain attributes in state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is:

  • don't save the updated HTML (which means parse the attributes) on every key stroke (weird things happen if you do so :) without mentioning the performance reasons) and only persist these changes onBlur
  • If the attributes change externally, this means something else changed the current block, will have to recompute the HTML again.
  • Each time the text mode changes the attributes we store them in state to compare with the new prop.

If you have another suggestion to achieve this, I'm all for it.

Copy link
Member

Choose a reason for hiding this comment

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

don't save the updated HTML (which means parse the attributes) on every key stroke (weird things happen if you do so :) without mentioning the performance reasons) and only persist these changes onBlur

This is fine, I understand we don't want to update the block until blur, so storing initial HTML makes sense. Doesn't seem to imply we need attributes to do so, though.

If the attributes change externally, this means something else changed the current block, will have to recompute the HTML again.

Each time the text mode changes the attributes we store them in state to compare with the new prop.

We can compare isEqual( this.props.attributes, nextProps.block.attributes ) in componentWillReceiveProps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right 👍

if ( action.type === 'TOGGLE_BLOCK_MODE' ) {
return {
...state,
[ action.uid ]: state[ action.uid ] && state[ action.uid ] === 'html' ? 'visual' : 'html',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a destructure could help clear this up a bit:

export function blocksMode( state = {}, action ) {
	const { type, uid } = action;
	if ( type === 'TOGGLE_BLOCK_MODE' ) {
		return {
			...state,
			[ uid ]: state[ uid ] && state[ uid ] === 'html' ? 'visual' : 'html',
		};
	}

	return state;
}

Or inside the condition if we want to avoid assignment for unhandled actions:

export function blocksMode( state = {}, action ) {
	if ( action.type === 'TOGGLE_BLOCK_MODE' ) {
		const { uid } = action;
		return {
			...state,
			[ uid ]: state[ uid ] && state[ uid ] === 'html' ? 'visual' : 'html',
		};
	}

	return state;
}

@youknowriad
Copy link
Contributor Author

After #2884 merge, I updated this and it's in the block's menu right now.

Also, What about the "invalid" nudge, any design thoughts or mockups on this @jasmussen?

@jasmussen
Copy link
Contributor

Also, What about the "invalid" nudge, any design thoughts or mockups on this @jasmussen?

I think that is probably best explored separately. But it could be something in this vein:

screen shot 2017-10-09 at 15 18 08

CC: @mtias as I know he had a bunch of thoughts on that.

@youknowriad
Copy link
Contributor Author

Nice design, so do you feel this is ready to go? what's left here?

@jasmussen
Copy link
Contributor

Nice design, so do you feel this is ready to go? what's left here?

I feel like this is ready to go, yes. A nice enhancement could be to have the "html mode" button have a "toggled" state — just like if it were a "bold" button. That is, dark square background, and white text on it. I was also thinking instead of having Trash and Configure float on the side, we could put them in a popout menu. But this is an enhancement we can do later on, I feel.

Thoughts, @mtias ? I think it's 👍 👍

@karmatosed
Copy link
Member

👍 from me and :shipit: :)

@afercia
Copy link
Contributor

afercia commented Oct 9, 2017

From an accessibility perspective, I have no objections if you want to implement an HTML mode that is basically a single contenteditable block.

I'd just like to remind you all that a "text mode" with a simple textarea is a must-have for accessibility, as it's the only guarantee it can be operated by all users with any device or assistive technology.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 9, 2017
@youknowriad youknowriad removed Needs Design Needs design efforts. Needs Tests labels Oct 10, 2017
@youknowriad
Copy link
Contributor Author

Merging this, should we open an issue for the updated design

@youknowriad youknowriad merged commit 6268099 into master Oct 10, 2017
@youknowriad youknowriad deleted the try/html-per-block branch October 10, 2017 08:21
@jasmussen
Copy link
Contributor

Merging this, should we open an issue for the updated design

I'm working on some enhancements here, will open a ticket as soon as I can.

DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants