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

RichText state structure for value manipulation #7890

Merged
merged 44 commits into from Oct 1, 2018

Conversation

@iseulde
Member

iseulde commented Jul 11, 2018

Description

Read about the functions for manipulation.

Why a different RichText internal state structure?

Currently RichText values is internally structured as tree of HTML nodes that can map to the displayed live DOM. When the value needs to be manipulated, we are either looking at the live DOM or the internal array structure.

Current parsing and serialisation

Raw parsed blocks have a blob of HTML as their content, from which attributes are sourced according to the block’s attribute schema. This is done by parsing the HTML with the browser. For RichText values, the in memory DOM tree is then converted to an array structure of the same shape. In the RichText editor, the values are applied by setting the HTML (element.innerHTML). On Input, the live DOM is converted as well to an array structure. Any manipulation that is done by us happens by manipulation the live DOM directly, or by manipulating the array structure, which is then set as HTML in the editor.

Proposed parsing and serialisation: controlled component

Ideally, we’d like to avoid manipulating the live DOM directly, be it though TinyMCE APIs or not. It is both very fragile (prone to errors) and not so easy to do. Say you’d like to automatically format some text that matches @user, and complete it based on suggestion, you’d have to merge all text children of element nodes in the tree before the selection to be able to search, split elements and text nodes in order to format the matching text, and move selection using the Selection API. Formatting etc. is similarly complex within TinyMCE. See https://github.com/tinymce/tinymce/tree/master/src/core/main/ts/fmt. A tree shape is not so easy to manipulate, wether it is the live DOM or an array structure. A flat structure would be easier, where text and formats are separated. I’m not proposing an alternative format for saving or displaying the data. HTML remains perfect to store the value, and the DOM needs to be used, in the browser, to display the data in a contentEditable container.

The best structure I’ve found was one where the text and formats are separated. The text would be just a string, which becomes easy to search. The formats are stored in a sparse array of the same length as the text. Each index of the array maps to a character in the string, and formats are reference to a unique format variation. RichText values become very easy to manipulate like this, imagine for example splitting, merging, slicing, splicing, applying and removing formats. Empty checking is simply a matter of checking the string. Things like an empty HTML element and empty text nodes are not possible.

Example:

<p>one <em><a href="https://w.org">two</a></em></p>
// All formats are referenced, this is to illustrate.
const format1 = { name: 'italic' };
const format2 = { name: 'link', url: 'https://w.org' };

{
	formats: [
		,
		,
		,
		,
		[ format1, format2 ],
		[ format1, format2 ],
		[ format1, format2 ],
	],
	text: 'one two',
}

New formats could be registered with their own view:

<p>one <span class="mention">two</span></em></p>
const format1 = { name: 'mention' };
{
	formats: [
		,
		,
		,
		,
		[ format1 ],
		[ format1 ],
		[ format1 ],
	],
	text: 'one two',
}

We could also support some meta date here if we need to save extra information e.g. attributes or a user ID.

Because there is a textual representation, RichText instances could be searched and formatted easily, e.g. think about auto-formatting @mentions and marking pieces of text for spell checking or comments.

Another benefit is that there is only one right way to map this to HTML. Merging

const format1 = { name: 'italic' };

{
	formats: [
		[ format1 ],
		[ format1 ],
		[ format1 ],
	],
	text: 'one',
}
const format1 = { name: 'italic' };

{
	formats: [
		[ format1 ],
		[ format1 ],
		[ format1 ],
	],
	text: 'two',
}

will result in <em>onetwo</em>. Not <em>one</em><em>two</em>.

How has this been tested?

Gutenberg should load without errors. Test splitting and merging, formatting, inline tokens, text patterns, blocks transformations.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@brandonpayton

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Jul 15, 2018

Member

Instead I'd like to propose an alternative structure where the text content and formatting are separated.

Thanks so much for working on this. We really need it. It would allow us to style autocompletions like @-mentions based on a pattern match rather than modifying the actual content like #6577 does. This would be cleaner, more flexible, and less error-prone.

Member

brandonpayton commented Jul 15, 2018

Instead I'd like to propose an alternative structure where the text content and formatting are separated.

Thanks so much for working on this. We really need it. It would allow us to style autocompletions like @-mentions based on a pattern match rather than modifying the actual content like #6577 does. This would be cleaner, more flexible, and less error-prone.

@brandonpayton

This seems like a really good direction if some pattern transforms can be shared across all RichText instances for things like @-mentions. In general, the changes felt like they were simplifying. I like how this is consolidating text-related logic in rich-text-structure.

Show outdated Hide outdated blocks/api/parser.js
Show outdated Hide outdated blocks/api/rich-text-structure.js
Show outdated Hide outdated blocks/api/rich-text-structure.js
Show outdated Hide outdated blocks/api/rich-text-structure.js
Show outdated Hide outdated editor/components/rich-text/index.js
@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jul 27, 2018

Member

@brandonpayton I update the autocompleter to use the rich text structure. I think this simplifies it a lot, but it needs some more polishing for sure there. Also wondering if it should maybe be moved in the rich text folder...

Member

iseulde commented Jul 27, 2018

@brandonpayton I update the autocompleter to use the rich text structure. I think this simplifies it a lot, but it needs some more polishing for sure there. Also wondering if it should maybe be moved in the rich text folder...

@brandonpayton

I love these changes, and it's great to see the Autocomplete component simplified. Thank you.

Also wondering if it should maybe be moved in the rich text folder...

That seems like a good move to me. Autocomplete already had things that made it more editor-related than generic. Even before this PR, its implementation assumed wrapping something like a contenteditable and had the onReplace prop which implies use in a block context. IMHO, bringing it even closer to rich-text to work with a more consistent and reliable editor layer is a win.

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 1, 2018

Member

Left to do here:

  • Finish Autocompleter component. Cc @brandonpayton.
  • Make sure all unit and e2e tests pass.
  • Ensure deprecated block markup still works. Unfortunately almost none of these have tests... :(
  • Look into custom formatters and adjust it.
  • Look into table block. Ideally this shouldn't be one big RichText area, but one per cell. Either actions that are currently handled by the TinyMCE plugin are moved to the block registration, or the block should make use of nested blocks. Same for the list block, but that's not a blocker for this PR. Cc @matias for vision.
  • Work out how the format prop should works. Internally I think we should only use the RichText structure, but block authors should be able to get and set e.g. HTML. Cc @youknowriad.
  • Finish format registration.
  • Add API for registering formats, auto-completers, auto-transformers and patterns.
    • All formatting does is applying a format for a selection of text in a record.
    • Auto-completers look for what is being typed and list suggestions, which can then be inserted (just text, or format with text).
    • Auto-transformers replace any input immediately with something else. E.g. if you type a non breaking space, the character can be replaced with some other entity to make it visible. When you type @something, it will automatically be formatted as a mention, and lose its formatting if it no longer matches.
    • Text patterns transform some given text to another text with formatting. This can probably be merged with the above.
      I'm looking for a way to unify all of the above in a relative simple API.
Member

iseulde commented Aug 1, 2018

Left to do here:

  • Finish Autocompleter component. Cc @brandonpayton.
  • Make sure all unit and e2e tests pass.
  • Ensure deprecated block markup still works. Unfortunately almost none of these have tests... :(
  • Look into custom formatters and adjust it.
  • Look into table block. Ideally this shouldn't be one big RichText area, but one per cell. Either actions that are currently handled by the TinyMCE plugin are moved to the block registration, or the block should make use of nested blocks. Same for the list block, but that's not a blocker for this PR. Cc @matias for vision.
  • Work out how the format prop should works. Internally I think we should only use the RichText structure, but block authors should be able to get and set e.g. HTML. Cc @youknowriad.
  • Finish format registration.
  • Add API for registering formats, auto-completers, auto-transformers and patterns.
    • All formatting does is applying a format for a selection of text in a record.
    • Auto-completers look for what is being typed and list suggestions, which can then be inserted (just text, or format with text).
    • Auto-transformers replace any input immediately with something else. E.g. if you type a non breaking space, the character can be replaced with some other entity to make it visible. When you type @something, it will automatically be formatted as a mention, and lose its formatting if it no longer matches.
    • Text patterns transform some given text to another text with formatting. This can probably be merged with the above.
      I'm looking for a way to unify all of the above in a relative simple API.
@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 1, 2018

Member

`test` => <code>test</code> (without suggestions)
> test => quote block (without suggestions)
@m => <span …>@mention</span> (with suggestions)
@mention => <span …>@mention</span> (typed, no suggestions used)
[… similarly #, +, etc.]
:apple: => 🍎
/block => block (with suggestions, or just typed and enter)
Non-breaking space => wrap in tag
Add formatting that is only visible in the editor (comments, spell checking tags)
Inline tokens

Member

iseulde commented Aug 1, 2018

`test` => <code>test</code> (without suggestions)
> test => quote block (without suggestions)
@m => <span …>@mention</span> (with suggestions)
@mention => <span …>@mention</span> (typed, no suggestions used)
[… similarly #, +, etc.]
:apple: => 🍎
/block => block (with suggestions, or just typed and enter)
Non-breaking space => wrap in tag
Add formatting that is only visible in the editor (comments, spell checking tags)
Inline tokens

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 1, 2018

Member

I think something like this might be good:

[
	{
		format: 'bold',
		selector: 'strong',
		shortcuts: [
			{
				type: 'primary',
				key: 'b',
			},
		],
		button: {
			icon: 'editor-bold',
			title: __( 'Bold' ),
		},
	},
	{
		format: 'italic',
		selector: 'em',
		shortcuts: [
			{
				type: 'primary',
				key: 'i',
			},
		],
		button: {
			icon: 'editor-italic',
			title: __( 'Italic' ),
		},
	},
	{
		format: 'strikethrough',
		selector: 'del',
		shortcuts: [
			{
				type: 'access',
				key: 'd'
			}
		],
		button: {
			icon: 'editor-strikethrough',
			title: __( 'Strikethrough' ),
		},
	},
	{
		format: 'code',
		selector: 'code',
		shortcuts: [
			{
				type: 'access',
				key: 'x',
			},
		],
		match: /`([^`]+)`/,
		onMatch( { record, setRecord } ) {
			// Transform...
		},
	},
	{
		format: 'link',
		selector: 'a',
		attributes: {
			url: {
				source: 'attribute',
				attribute: 'href',
			},
		},
		shortcuts: ( format ) => {
			if ( format ) {
				return [
					{
						type: 'access',
						key: 's',
					},
				];
			}

			return [
				{
					type: 'primary',
					key: 'k',
				},
				{
					type: 'access',
					key: 'a',
				},
			];
		},
		button: ( format ) => {
			if ( format ) {
				return {
					icon: 'editor-unlink',
					title: __( 'Unlink' ),
				};
			}

			return {
				icon: 'admin-links',
				title: __( 'Link' ),
			};
		},
		create( { record, setRecord } ) {
			// Link creation UI...
		},
	},
	{
		format: 'image',
		selector: 'img',
		attributes: {
			url: {
				source: 'attribute',
				attribute: 'src',
			},
		},
		inserter: {
			icon: <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M4 16h10c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2H4c-1.1 0-2 .9-2 2v9c0 1.1.9 2 2 2zM4 5h10v9H4V5zm14 9v2h4v-2h-4zM2 20h20v-2H2v2zm6.4-8.8L7 9.4 5 12h8l-2.6-3.4-2 2.6z" /></svg>,
			title: __( 'Inline Image' ),
		},
		create( { record, setRecord } ) {
			return (
				<MediaUpload
					type="image"
					onSelect={ ( media ) => {} }
					onClose={ () => {} }
					render={ ( { open } ) => {
						open();
						return null;
					} }
				/>
			);
		}
	},
	{
		format: 'mention',
		selector: 'a.wp-mention',
		attributes: {
			id: {
				source: 'attribute',
				attribute: 'data-id',
			},
		},
		inserter: {
			icon: '@',
			title: __( 'Mention' ),
		},
		textShape: /@\w+/,
		// Auto format any text matching this pattern.
		// Also auto remove format if the text no longer matches.
		match: /(?:\s|^)(@\w+)(?:\s|$)/,
		onMatch( { record, setRecord } ) {
			// List suggestions.
		},
	}
]

Slash inserter is something that can just stay internal I think, as it's special to only work on a default block and it should match the full text.

Member

iseulde commented Aug 1, 2018

I think something like this might be good:

[
	{
		format: 'bold',
		selector: 'strong',
		shortcuts: [
			{
				type: 'primary',
				key: 'b',
			},
		],
		button: {
			icon: 'editor-bold',
			title: __( 'Bold' ),
		},
	},
	{
		format: 'italic',
		selector: 'em',
		shortcuts: [
			{
				type: 'primary',
				key: 'i',
			},
		],
		button: {
			icon: 'editor-italic',
			title: __( 'Italic' ),
		},
	},
	{
		format: 'strikethrough',
		selector: 'del',
		shortcuts: [
			{
				type: 'access',
				key: 'd'
			}
		],
		button: {
			icon: 'editor-strikethrough',
			title: __( 'Strikethrough' ),
		},
	},
	{
		format: 'code',
		selector: 'code',
		shortcuts: [
			{
				type: 'access',
				key: 'x',
			},
		],
		match: /`([^`]+)`/,
		onMatch( { record, setRecord } ) {
			// Transform...
		},
	},
	{
		format: 'link',
		selector: 'a',
		attributes: {
			url: {
				source: 'attribute',
				attribute: 'href',
			},
		},
		shortcuts: ( format ) => {
			if ( format ) {
				return [
					{
						type: 'access',
						key: 's',
					},
				];
			}

			return [
				{
					type: 'primary',
					key: 'k',
				},
				{
					type: 'access',
					key: 'a',
				},
			];
		},
		button: ( format ) => {
			if ( format ) {
				return {
					icon: 'editor-unlink',
					title: __( 'Unlink' ),
				};
			}

			return {
				icon: 'admin-links',
				title: __( 'Link' ),
			};
		},
		create( { record, setRecord } ) {
			// Link creation UI...
		},
	},
	{
		format: 'image',
		selector: 'img',
		attributes: {
			url: {
				source: 'attribute',
				attribute: 'src',
			},
		},
		inserter: {
			icon: <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M4 16h10c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2H4c-1.1 0-2 .9-2 2v9c0 1.1.9 2 2 2zM4 5h10v9H4V5zm14 9v2h4v-2h-4zM2 20h20v-2H2v2zm6.4-8.8L7 9.4 5 12h8l-2.6-3.4-2 2.6z" /></svg>,
			title: __( 'Inline Image' ),
		},
		create( { record, setRecord } ) {
			return (
				<MediaUpload
					type="image"
					onSelect={ ( media ) => {} }
					onClose={ () => {} }
					render={ ( { open } ) => {
						open();
						return null;
					} }
				/>
			);
		}
	},
	{
		format: 'mention',
		selector: 'a.wp-mention',
		attributes: {
			id: {
				source: 'attribute',
				attribute: 'data-id',
			},
		},
		inserter: {
			icon: '@',
			title: __( 'Mention' ),
		},
		textShape: /@\w+/,
		// Auto format any text matching this pattern.
		// Also auto remove format if the text no longer matches.
		match: /(?:\s|^)(@\w+)(?:\s|$)/,
		onMatch( { record, setRecord } ) {
			// List suggestions.
		},
	}
]

Slash inserter is something that can just stay internal I think, as it's special to only work on a default block and it should match the full text.

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 7, 2018

Member

I modified the table block (to use nested blocks and work with the RichText structure). It won't look nice yet but it will display. @jasmussen Would it be possible to have a look at the table block in this PR? I'm looking to get the nested table rows and cells displayed nicely. :)

Unit tests now pass as well.

Member

iseulde commented Aug 7, 2018

I modified the table block (to use nested blocks and work with the RichText structure). It won't look nice yet but it will display. @jasmussen Would it be possible to have a look at the table block in this PR? I'm looking to get the nested table rows and cells displayed nicely. :)

Unit tests now pass as well.

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 7, 2018

Contributor

I'll try to take a look tomorrow!

Contributor

jasmussen commented Aug 7, 2018

I'll try to take a look tomorrow!

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor
I'm getting this error loading the demo content:
react-dom.min.82e21c65.js:110 TypeError: _wordpress_editor__WEBPACK_IMPORTED_MODULE_8__.RichText.isEmpty is not a function
    at save (index.js:249)
    at getSaveElement (validation.js:133)
    at getSaveContent (validation.js:197)
    at addParsedDifference (index.js?ver=1533710963:31946)
    at runHooks (createRunHook.js:56)
    at getBlockAttributes (index.js?ver=1533711214:13802)
    at createBlockWithFallback (parser.js:79)
    at parser.js:104
    at Array.reduce (<anonymous>)
    at parser.js:103
bg @ react-dom.min.82e21c65.js:110
index.js:249 Uncaught (in promise) TypeError: _wordpress_editor__WEBPACK_IMPORTED_MODULE_8__.RichText.isEmpty is not a function
    at save (index.js:249)
    at getSaveElement (validation.js:133)
    at getSaveContent (validation.js:197)
    at addParsedDifference (index.js?ver=1533710963:31946)
    at runHooks (createRunHook.js:56)
    at getBlockAttributes (index.js?ver=1533711214:13802)
    at createBlockWithFallback (parser.js:79)
    at parser.js:104
    at Array.reduce (<anonymous>)
    at parser.js:103

Also, not sure if it's just this branch but the tips keep showing on every reload.

Scratch that, see below.

Contributor

jasmussen commented Aug 8, 2018

I'm getting this error loading the demo content:
react-dom.min.82e21c65.js:110 TypeError: _wordpress_editor__WEBPACK_IMPORTED_MODULE_8__.RichText.isEmpty is not a function
    at save (index.js:249)
    at getSaveElement (validation.js:133)
    at getSaveContent (validation.js:197)
    at addParsedDifference (index.js?ver=1533710963:31946)
    at runHooks (createRunHook.js:56)
    at getBlockAttributes (index.js?ver=1533711214:13802)
    at createBlockWithFallback (parser.js:79)
    at parser.js:104
    at Array.reduce (<anonymous>)
    at parser.js:103
bg @ react-dom.min.82e21c65.js:110
index.js:249 Uncaught (in promise) TypeError: _wordpress_editor__WEBPACK_IMPORTED_MODULE_8__.RichText.isEmpty is not a function
    at save (index.js:249)
    at getSaveElement (validation.js:133)
    at getSaveContent (validation.js:197)
    at addParsedDifference (index.js?ver=1533710963:31946)
    at runHooks (createRunHook.js:56)
    at getBlockAttributes (index.js?ver=1533711214:13802)
    at createBlockWithFallback (parser.js:79)
    at parser.js:104
    at Array.reduce (<anonymous>)
    at parser.js:103

Also, not sure if it's just this branch but the tips keep showing on every reload.

Scratch that, see below.

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor

Ignore my previous comment. The good ol'e rm -rf ./node_modules followed by an npm install fixed it right up.

Contributor

jasmussen commented Aug 8, 2018

Ignore my previous comment. The good ol'e rm -rf ./node_modules followed by an npm install fixed it right up.

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor

I took a look at the table block in this branch, and getting this to behave like the frontend is going to be very very difficult due to all the extra divs that each block adds.

There is a new CSS property incoming, I can't recall its name, I read about it on CSS Tricks a while back. Supposedly you can apply this CSS property to an element, to make it sort of a "passthrough" element, basically make it as if it doesn't exist, and the immediate child becomes the direct child. My Google-fu is failing me, and supposedly the browser support isn't there anyway.

But as it stands, I can't see an easy path forward to make this work.

If we had that CSS property, we could relatively easily map this:

<table>
	<tr>
		<td></td>
	</tr>
	<tr>
		<td></td>
	</tr>
</table>

to this:

<div class="wp-block-table">
	<div class="wp-block-table-row">
		<div class="wp-block-table-cell"></div>
	</div>
	<div class="wp-block-table-row">
		<div class="wp-block-table-cell"></div>
	</div>
</div>

That markup above is relatively easily made to behave as a proper table with a few displays thrown in. But with the current markup, I honestly don't see a clean path forward to make a table made of nested blocks work.

Do we need to convert the table to use nested blocks right now? Is there another way?

Contributor

jasmussen commented Aug 8, 2018

I took a look at the table block in this branch, and getting this to behave like the frontend is going to be very very difficult due to all the extra divs that each block adds.

There is a new CSS property incoming, I can't recall its name, I read about it on CSS Tricks a while back. Supposedly you can apply this CSS property to an element, to make it sort of a "passthrough" element, basically make it as if it doesn't exist, and the immediate child becomes the direct child. My Google-fu is failing me, and supposedly the browser support isn't there anyway.

But as it stands, I can't see an easy path forward to make this work.

If we had that CSS property, we could relatively easily map this:

<table>
	<tr>
		<td></td>
	</tr>
	<tr>
		<td></td>
	</tr>
</table>

to this:

<div class="wp-block-table">
	<div class="wp-block-table-row">
		<div class="wp-block-table-cell"></div>
	</div>
	<div class="wp-block-table-row">
		<div class="wp-block-table-cell"></div>
	</div>
</div>

That markup above is relatively easily made to behave as a proper table with a few displays thrown in. But with the current markup, I honestly don't see a clean path forward to make a table made of nested blocks work.

Do we need to convert the table to use nested blocks right now? Is there another way?

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 8, 2018

Member

@jasmussen The problem with the table block for this PR is that it doesn't fit with the structure it proposes. Either the RichText area needs to be a single line of text with formatting, or multiple lines of text with formatting (such as quote and list atm). I'm guessing that both quote and list will also eventually use nested block, which would simplify RichText further, but it's no problem at the moment to accommodate them. With the table block, however, there are many nested elements in the RichText area. The component as it is now is not really made for it either, but it is certainly incompatible with this structure. Either the table block needs to use RichText areas for each cell (as nested blocks, or in one block), or we could maybe create a temporary special EditableTable component or something. One block with multiple RichText areas I've also tried before but all the TinyMCE functionality like inserting/deleting columns and row, and moving them, needs to be reimplemented. By using nested blocks I though we could reuse some existing functionality. Also in the long term it may be weird if controls inside the table block are different from nested blocks, maybe better to have just one API for it.

Member

iseulde commented Aug 8, 2018

@jasmussen The problem with the table block for this PR is that it doesn't fit with the structure it proposes. Either the RichText area needs to be a single line of text with formatting, or multiple lines of text with formatting (such as quote and list atm). I'm guessing that both quote and list will also eventually use nested block, which would simplify RichText further, but it's no problem at the moment to accommodate them. With the table block, however, there are many nested elements in the RichText area. The component as it is now is not really made for it either, but it is certainly incompatible with this structure. Either the table block needs to use RichText areas for each cell (as nested blocks, or in one block), or we could maybe create a temporary special EditableTable component or something. One block with multiple RichText areas I've also tried before but all the TinyMCE functionality like inserting/deleting columns and row, and moving them, needs to be reimplemented. By using nested blocks I though we could reuse some existing functionality. Also in the long term it may be weird if controls inside the table block are different from nested blocks, maybe better to have just one API for it.

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 8, 2018

Member

(I'm also not quite sure why the added div would make it hard to create columns. Can't it just work lik ethe column block, where the cells behave like columns in the column block? The rows can just stack vertically on each other, no need to do anything there.)

Member

iseulde commented Aug 8, 2018

(I'm also not quite sure why the added div would make it hard to create columns. Can't it just work lik ethe column block, where the cells behave like columns in the column block? The rows can just stack vertically on each other, no need to do anything there.)

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor

To be clear I'm not objecting to the technical side, just noting that it is very very difficult in the implementation. There's not just one added div, there are a whole slew of added divs, and in the end I need one element to act as table, one element to act as a row, and the direct descendant of that to act as a cell. If the descendant is not a direct descendant, that's when it gets really difficult. It's tricky enough for column, but even that's caused headaches. For tables, it's extra hard.

I can give it another shot, for sure, but I'm not 100% sure I can get this to work :(

The alternative is to emulate the appearance of a table, without it actually being a table. That's certainly doable but it's tricky too.

Contributor

jasmussen commented Aug 8, 2018

To be clear I'm not objecting to the technical side, just noting that it is very very difficult in the implementation. There's not just one added div, there are a whole slew of added divs, and in the end I need one element to act as table, one element to act as a row, and the direct descendant of that to act as a cell. If the descendant is not a direct descendant, that's when it gets really difficult. It's tricky enough for column, but even that's caused headaches. For tables, it's extra hard.

I can give it another shot, for sure, but I'm not 100% sure I can get this to work :(

The alternative is to emulate the appearance of a table, without it actually being a table. That's certainly doable but it's tricky too.

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 8, 2018

Member

@jasmussen Okay, so no grid tricks to use here? :D I think I'll just revert it to a TinyMCE instance for now that is separated from RichText, but that also means things like extending, formatting etc. won't work unless specifically added, which may be okay for now.

Member

iseulde commented Aug 8, 2018

@jasmussen Okay, so no grid tricks to use here? :D I think I'll just revert it to a TinyMCE instance for now that is separated from RichText, but that also means things like extending, formatting etc. won't work unless specifically added, which may be okay for now.

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor

If there's any way we can do this in multiple pull requests I think that would be best. I don't have any solid and simple ideas for how to get it to work otherwise

Contributor

jasmussen commented Aug 8, 2018

If there's any way we can do this in multiple pull requests I think that would be best. I don't have any solid and simple ideas for how to get it to work otherwise

@iseulde

This comment was marked as outdated.

Show comment
Hide comment
@iseulde

iseulde Aug 8, 2018

Member

Okay, let me tinker on some alternative. And yeah I now realise that stuff like auto table layout wouldn't work... :/ Thanks for the help here @jasmussen!

Member

iseulde commented Aug 8, 2018

Okay, let me tinker on some alternative. And yeah I now realise that stuff like auto table layout wouldn't work... :/ Thanks for the help here @jasmussen!

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 8, 2018

Contributor

Any time, and I'll keep thinking.

Contributor

jasmussen commented Aug 8, 2018

Any time, and I'll keep thinking.

@brandonpayton

This comment was marked as outdated.

Show comment
Hide comment
@brandonpayton

brandonpayton Aug 9, 2018

Member

There is a new CSS property incoming, I can't recall its name, I read about it on CSS Tricks a while back. Supposedly you can apply this CSS property to an element, to make it sort of a "passthrough" element, basically make it as if it doesn't exist.

Hi @jasmussen, a friend mentioned this to me today. I think it's display: contents. Unfortunately, not all our supported browsers support the feature, and apparently, there are also accessibility issues with current implementations where the setting causes elements to be entirely removed from the accessibility tree. 🙃

Member

brandonpayton commented Aug 9, 2018

There is a new CSS property incoming, I can't recall its name, I read about it on CSS Tricks a while back. Supposedly you can apply this CSS property to an element, to make it sort of a "passthrough" element, basically make it as if it doesn't exist.

Hi @jasmussen, a friend mentioned this to me today. I think it's display: contents. Unfortunately, not all our supported browsers support the feature, and apparently, there are also accessibility issues with current implementations where the setting causes elements to be entirely removed from the accessibility tree. 🙃

@jasmussen

This comment was marked as outdated.

Show comment
Hide comment
@jasmussen

jasmussen Aug 9, 2018

Contributor

Yes! That's the one! Thanks, now that rogue process in my brain can be deallocated.

And I did sort of assume that it wouldn't work for us. But the alternative, Shadow DOM, might be even less supported.

Contributor

jasmussen commented Aug 9, 2018

Yes! That's the one! Thanks, now that rogue process in my brain can be deallocated.

And I did sort of assume that it wouldn't work for us. But the alternative, Shadow DOM, might be even less supported.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 1, 2018

Member

@mcsf I renamed the package and the source, I also updated the deprecation messages, and made it so that you can still pass a string with createBlock when using the new source (as a shortcut for create( { text: ... } ) to create the attribute value, which may be easier for templates.

Member

iseulde commented Oct 1, 2018

@mcsf I renamed the package and the source, I also updated the deprecation messages, and made it so that you can still pass a string with createBlock when using the new source (as a shortcut for create( { text: ... } ) to create the attribute value, which may be easier for templates.

@mcsf

mcsf approved these changes Oct 1, 2018

Thank you very much for the great work and patience. I think we have all the things needed in here to merge. 🎉

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 1, 2018

Member

Thanks @mcsf! I will be watching closely for any problems.

Member

iseulde commented Oct 1, 2018

Thanks @mcsf! I will be watching closely for any problems.

@iseulde iseulde merged commit 78d1254 into master Oct 1, 2018

2 checks passed

codecov/project 49.3% (+0.73%) compared to 7509ace
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Oct 1, 2018

@iseulde iseulde deleted the try/rich-text-record branch Oct 1, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 1, 2018

Contributor

Fantastic @iseulde

Contributor

youknowriad commented Oct 1, 2018

Fantastic @iseulde

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 1, 2018

Member

So good!

nyan

Member

tofumatt commented Oct 1, 2018

So good!

nyan

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 2, 2018

Member

I noticed a minor typo here☝️

Member

gziolo commented on test/e2e/test-plugins/deprecated-node-matcher/index.js in 94352e7 Oct 2, 2018

I noticed a minor typo here☝️

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 2, 2018

Member

Ooops... From search-replacing :)

Member

iseulde replied Oct 2, 2018

Ooops... From search-replacing :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 2, 2018

Member

Love the changes added in 94352e7. Ace work on this PR @iseulde 👏 Thanks for being patient with all our feedback 😃

Member

gziolo commented Oct 2, 2018

Love the changes added in 94352e7. Ace work on this PR @iseulde 👏 Thanks for being patient with all our feedback 😃

@hypest

This comment has been minimized.

Show comment
Hide comment
@hypest

hypest Oct 2, 2018

Contributor

🎉 Thanks for the effort here @iseulde and thanks for "minding the mobile" and working closely with the mobile team! 🙇

Contributor

hypest commented Oct 2, 2018

🎉 Thanks for the effort here @iseulde and thanks for "minding the mobile" and working closely with the mobile team! 🙇

@iseulde iseulde referenced this pull request Oct 3, 2018

Closed

RichText State Structure #771

iseulde added a commit that referenced this pull request Oct 3, 2018

"react-native": "src/index",
"dependencies": {
"@babel/runtime": "^7.0.0",
"@wordpress/escape-html": "file:../escape-html",

This comment has been minimized.

@aduth

aduth Oct 3, 2018

Member

This introduces uncommitted changes to the local branch after npm install. Please review #10234

@aduth

aduth Oct 3, 2018

Member

This introduces uncommitted changes to the local branch after npm install. Please review #10234

This comment has been minimized.

@iseulde

iseulde Oct 3, 2018

Member

Do you mean package lock? See #10304.

@iseulde

iseulde Oct 3, 2018

Member

Do you mean package lock? See #10304.

iseulde added a commit that referenced this pull request Oct 3, 2018

export { insert } from './insert';
export { slice } from './slice';
export { split } from './split';
export { apply, toDom as unstableToDom } from './to-dom';

This comment has been minimized.

@aduth
@aduth

aduth Oct 5, 2018

Member
@ktmn

This comment has been minimized.

Show comment
Hide comment
@ktmn

ktmn Oct 9, 2018

Quick question about this rich text, previously if I had a template like this:

<wp.editor.InnerBlocks template={[
	['core/paragraph', {
		content: ['Default content'],
	}],
]} />

Now with array it doesn't work, string does:

<wp.editor.InnerBlocks template={[
	['core/paragraph', {
		content: 'Default content',
	}],
]} />

And adding a line break, for example, worked like this:

<InnerBlocks template={[
    ['core/paragraph', {
        content: [
            'Hello',
            {
                type: 'br',
                props: {
                    children: [],
                },
            },
            'world',
        ],
    }],
]} />

How would I do that now? I tried content: wp.richText.create('Hello<br>world') but that was not it. Do I need like an object with formats and text or something?

The docs don't really say much, create( ?input: Element | string, ?range: Range, ?multilineTag: string, ?settings: Object ): Object, whats the range, what are the settings?

ktmn commented Oct 9, 2018

Quick question about this rich text, previously if I had a template like this:

<wp.editor.InnerBlocks template={[
	['core/paragraph', {
		content: ['Default content'],
	}],
]} />

Now with array it doesn't work, string does:

<wp.editor.InnerBlocks template={[
	['core/paragraph', {
		content: 'Default content',
	}],
]} />

And adding a line break, for example, worked like this:

<InnerBlocks template={[
    ['core/paragraph', {
        content: [
            'Hello',
            {
                type: 'br',
                props: {
                    children: [],
                },
            },
            'world',
        ],
    }],
]} />

How would I do that now? I tried content: wp.richText.create('Hello<br>world') but that was not it. Do I need like an object with formats and text or something?

The docs don't really say much, create( ?input: Element | string, ?range: Range, ?multilineTag: string, ?settings: Object ): Object, whats the range, what are the settings?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 9, 2018

Contributor

@ktmn We're still in the process of ensuring the best upgrade experience without breakage for existing templates. See #10370

Contributor

youknowriad commented Oct 9, 2018

@ktmn We're still in the process of ensuring the best upgrade experience without breakage for existing templates. See #10370

@ktmn

This comment has been minimized.

Show comment
Hide comment
@ktmn

ktmn Oct 10, 2018

@youknowriad Ok I see, thanks. Another thing that seems broken for me is when a block previously had an attribute

content: {
	type: 'array',
},

that is used in wp.editor.RichText then it throws

Do all the richtext attribute types need to be changed, and if so, to what type? And do they need a default value? I tried rendering wp.editor.RichText without any value prop and it still threw it, so does it need something from me?

Basically I shouldn't worry about current version of Gutenberg at all, it's supposed to be broken? I wanted to develop some blocks that I put aside waiting for the non-paragraph block inserter, now that it's merged I grabbed the latest version but I should have installed something earlier, this is completely WIP?

ktmn commented Oct 10, 2018

@youknowriad Ok I see, thanks. Another thing that seems broken for me is when a block previously had an attribute

content: {
	type: 'array',
},

that is used in wp.editor.RichText then it throws

Do all the richtext attribute types need to be changed, and if so, to what type? And do they need a default value? I tried rendering wp.editor.RichText without any value prop and it still threw it, so does it need something from me?

Basically I shouldn't worry about current version of Gutenberg at all, it's supposed to be broken? I wanted to develop some blocks that I put aside waiting for the non-paragraph block inserter, now that it's merged I grabbed the latest version but I should have installed something earlier, this is completely WIP?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 10, 2018

Contributor

Basically I shouldn't worry about current version of Gutenberg at all, it's supposed to be broken?

Short answer, yes, we're working on it. The migration path is not fully decided.

Contributor

youknowriad commented Oct 10, 2018

Basically I shouldn't worry about current version of Gutenberg at all, it's supposed to be broken?

Short answer, yes, we're working on it. The migration path is not fully decided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment