RichText: List: Fix indent/outdent by ellatrix · Pull Request #12667 · WordPress/gutenberg

ellatrix · 2018-12-07T05:26:12Z

Description

Currently in some cases indenting and outdenting list items is broken.

This PR fixes some issues and uses the RichText value directly instead of using the editor commands.

How has this been tested?

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.

ellatrix · 2019-01-24T18:06:58Z

I'm still adding inline documentation for these changes.

aduth

Functionally, it seems to work quite well.

The code could very much use some commenting, as it's difficult to follow.

lib/packages-dependencies.php

packages/editor/src/components/rich-text/list-edit.js

aduth · 2019-01-24T18:22:31Z

packages/rich-text/src/change-list-type.js

+
+	let changed;
+
+	for ( let index = startIndex + 1 || 0; index < length; index++ ) {


When would || 0 ever take effect? The only situation I see is when the left-hand is 0 which... would produce the same result anyways.

I think startIndex could be undefined. In any case needs commenting.

packages/rich-text/src/change-list-type.js

packages/rich-text/src/indent-list-items.js

ellatrix · 2019-01-24T18:36:28Z

The code could very much use some commenting, as it's difficult to follow.

Very well aware :) That's why I commented earlier and it's still a WIP.

youknowriad · 2019-01-25T16:01:37Z

I had this error once when I was using the indent/outdent buttons.
I still don't know the exact steps to reproduce but thought I'd share.

youknowriad · 2019-01-25T16:05:50Z

Not sure if easy, but would love a fix for "Shift + Enter" but it could be good for a separate PR too.

Aside the error that I can't reproduce anymore (fine to ignore if you don't know the potential cause), everything is good when using the list block.

mcsf

So far testing looks good. I've done a first pass as I familiarise myself with this. Will be back. :)

packages/rich-text/src/test/change-list-type.js

mcsf · 2019-01-25T16:17:48Z

packages/rich-text/src/indent-list-items.js

+/**
+ * Indents any selected list items if possible.
+ *
+ * @param {Object} value      Value to change.


Do we have a type defined in JSDoc for rich text? If not, should we?

No, not so far. Perhaps we can look into that separately.

mcsf · 2019-01-25T16:24:27Z

packages/editor/src/components/rich-text/list-edit.js

+	tagName,
+	value,
+	onChange,
+} ) => (


Slightly tangential to the PR, but I would've expected the relationship between RichText and the List block to be more like:

List#edit = ( … ) => <ListEdit> <RichText /> </ListEdit>

rather than let RichText conditionally render ListEdit among its children based its own state and props (isSelected && this.multilineTag === 'li'). It feels like a strange burden on RichText and adds some indirection that reminds me of React/Backbone mixins, since List#edit has to remember to pass onTagNameChange to RichText. Also, I wonder why we need to generalise onTagNameChange instead of letting ListEdit implement specific logic for ul and ol.

Sounds good to rethink, but I feel like it might be better to do separately because we leave most of this unchanged.

aduth · 2019-01-25T18:15:11Z

lib/client-assets.php

 			gutenberg_override_script( 'wp-tinymce', includes_url( 'js/tinymce/' ) . "plugins/compat3x/plugin{$suffix}.js", array( 'wp-tinymce-root' ), $tinymce_version );
 		}
-
-		gutenberg_override_script( 'wp-tinymce-lists', includes_url( 'js/tinymce/' ) . "plugins/lists/plugin{$suffix}.js", array( 'wp-tinymce' ), $tinymce_version );


FYI: This line will separately become removed in #13466.

What's the process of removing core PHP related to Gutenberg PRs?

What's the process of removing core PHP related to Gutenberg PRs?

I'm currently gutting most of the PHP, so that we'll get to a point where Gutenberg PHP is primarily just overrides to the default core scripts and block implementations.

See also:

https://github.com/WordPress/gutenberg/projects/23

Remove PHP code merged into WordPress 5.0 #11015

youknowriad · 2019-01-28T14:16:17Z

I'm noticing some issues on Firefox.

Backspace is behaving weirdly, especially for indented lists
Sometimes I get this error

Sometimes, typing is stuck, for instance. Create this list:

- level 1
     - level 2

and then put the caret after "level 1", type Enter and try typing anything.

ellatrix · 2019-01-28T15:55:13Z

Backspace is behaving weirdly, especially for indented lists

When is it behaving weirdly. Steps to reproduce? Does the same thing happen on master? Worth noting that the PR shouldn't affect enter/backspace behaviour as it doesn't touch that logic.

Sometimes I get this error

Same as above

Sometimes, typing is stuck, for instance. Create this list:

Happens for master too. Let's look separately.

youknowriad

I confirm I have the same list block weirdness in master. Let's follow up on that separately. Using the list block is not very good in Firefox right now.

ellatrix · 2019-01-28T16:24:01Z

Thanks @youknowriad! I'll follow up with fixes for enter/backspace.

* RichText: List: use value to indent/outdent * Add outdent * Support multi outdent * Hold one reference per format * Keep list formats when indenting to new index * Remove unneeded parameter * Rename * Add logic to change list type * Add tests * Add e2e tests * Add some basic docs. Clean up. * Remove duplicate wp-tinymce dependency * Clean up * Add more docs, fix getSelectedListNode * Put duplicate text values in constant

ellatrix added [Status] In Progress Tracking issues with work in progress [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Dec 7, 2018

ellatrix force-pushed the try/list-use-value-indent-outdent branch from 25c3046 to 794c90e Compare December 8, 2018 16:51

ellatrix force-pushed the try/list-use-value-indent-outdent branch from 794c90e to 2f4b56b Compare January 9, 2019 12:07

aduth reviewed Jan 24, 2019

View reviewed changes

ellatrix force-pushed the try/list-use-value-indent-outdent branch from b008ba8 to 360e6dc Compare January 25, 2019 08:23

mcsf reviewed Jan 25, 2019

View reviewed changes

aduth reviewed Jan 25, 2019

View reviewed changes

ellatrix added 15 commits January 28, 2019 11:32

RichText: List: use value to indent/outdent

2fff02f

Add outdent

fffaa50

Support multi outdent

7d74d99

Hold one reference per format

42f9d86

Keep list formats when indenting to new index

72e7c15

Remove unneeded parameter

e768938

Rename

c55934a

Add logic to change list type

13139f3

Add tests

699e6d5

Add e2e tests

b84f0b9

Add some basic docs. Clean up.

b94e41d

Remove duplicate wp-tinymce dependency

0ab1e63

Clean up

6c77f19

Add more docs, fix getSelectedListNode

a205d8f

Put duplicate text values in constant

e4dc45d

ellatrix force-pushed the try/list-use-value-indent-outdent branch from ca99c91 to e4dc45d Compare January 28, 2019 10:33

youknowriad approved these changes Jan 28, 2019

View reviewed changes

ellatrix merged commit 32547dd into master Jan 28, 2019

ellatrix deleted the try/list-use-value-indent-outdent branch January 28, 2019 16:25

youknowriad added this to the 5.0 (Gutenberg) milestone Jan 28, 2019

ellatrix mentioned this pull request Jan 29, 2019

RichText: List: Fix outdent with children #13559

Merged

5 tasks

ellatrix modified the milestones: 5.0 (Gutenberg), WordPress 5.1.x Jan 29, 2019

youknowriad modified the milestones: WordPress 5.1.x, 5.0 (Gutenberg) Jan 29, 2019

This was referenced Feb 4, 2019

Bump plugin version to 5.0.0-rc.1 #13652

Merged

Bump plugin version to 5.0.0 #13689

Closed

Bump plugin version to 5.0.0 #13692

Merged

ellatrix mentioned this pull request Feb 10, 2019

List block: deleting an empty bulled folds the previous one too #13596

Closed


		let changed;

		for ( let index = startIndex + 1 \|\| 0; index < length; index++ ) {

Conversation

ellatrix commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

ellatrix commented Jan 24, 2019

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellatrix commented Jan 24, 2019

Uh oh!

youknowriad commented Jan 25, 2019

Uh oh!

youknowriad commented Jan 25, 2019

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Jan 28, 2019

Uh oh!

ellatrix commented Jan 28, 2019

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ellatrix commented Dec 7, 2018 •

edited

Loading