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: List: Fix indent/outdent #12667

Merged
merged 15 commits into from Jan 28, 2019

Conversation

Projects
None yet
4 participants
@iseulde
Copy link
Member

iseulde commented Dec 7, 2018

Description

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

list-mce

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

list-indent

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.

@iseulde iseulde force-pushed the try/list-use-value-indent-outdent branch from 25c3046 to 794c90e Dec 8, 2018

@iseulde iseulde force-pushed the try/list-use-value-indent-outdent branch from 794c90e to 2f4b56b Jan 9, 2019

@iseulde

This comment has been minimized.

Copy link
Member Author

iseulde commented Jan 24, 2019

I'm still adding inline documentation for these changes.

@aduth
Copy link
Member

aduth left a comment

Functionally, it seems to work quite well.

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

Show resolved Hide resolved lib/packages-dependencies.php Outdated
Show resolved Hide resolved packages/editor/src/components/rich-text/list-edit.js
Show resolved Hide resolved packages/editor/src/components/rich-text/list-edit.js Outdated

let changed;

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

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

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.

This comment has been minimized.

@iseulde

iseulde Jan 24, 2019

Author Member

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

Show resolved Hide resolved packages/rich-text/src/change-list-type.js Outdated
Show resolved Hide resolved packages/rich-text/src/indent-list-items.js Outdated
Show resolved Hide resolved packages/rich-text/src/indent-list-items.js Outdated
@iseulde

This comment has been minimized.

Copy link
Member Author

iseulde commented Jan 24, 2019

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.

@iseulde iseulde force-pushed the try/list-use-value-indent-outdent branch from b008ba8 to 360e6dc Jan 25, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 25, 2019

capture d ecran 2019-01-25 a 5 00 51 pm

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

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 25, 2019

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
Copy link
Contributor

mcsf left a comment

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

Show resolved Hide resolved packages/rich-text/src/test/change-list-type.js Outdated
/**
* Indents any selected list items if possible.
*
* @param {Object} value Value to change.

This comment has been minimized.

@mcsf

mcsf Jan 25, 2019

Contributor

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

This comment has been minimized.

@iseulde

iseulde Jan 28, 2019

Author Member

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

tagName,
value,
onChange,
} ) => (

This comment has been minimized.

@mcsf

mcsf Jan 25, 2019

Contributor

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.

This comment has been minimized.

@iseulde

iseulde Jan 28, 2019

Author Member

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

This comment has been minimized.

@mcsf

mcsf Jan 28, 2019

Contributor

Agreed

@@ -87,8 +87,6 @@ function register_tinymce_scripts() {
gutenberg_override_script( 'wp-tinymce-root', includes_url( 'js/tinymce/' ) . "tinymce{$mce_suffix}.js", array(), $tinymce_version );
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 );

This comment has been minimized.

@aduth

aduth Jan 25, 2019

Member

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

This comment has been minimized.

@iseulde

iseulde Jan 28, 2019

Author Member

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

This comment has been minimized.

@aduth

aduth Jan 28, 2019

Member

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:

@iseulde iseulde force-pushed the try/list-use-value-indent-outdent branch from ca99c91 to e4dc45d Jan 28, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 28, 2019

I'm noticing some issues on Firefox.

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

capture d ecran 2019-01-28 a 3 13 55 pm

  • 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.

@iseulde

This comment has been minimized.

Copy link
Member Author

iseulde commented Jan 28, 2019

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
Copy link
Contributor

youknowriad left a comment

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.

@iseulde

This comment has been minimized.

Copy link
Member Author

iseulde commented Jan 28, 2019

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

@iseulde iseulde merged commit 32547dd into master Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/list-use-value-indent-outdent branch Jan 28, 2019

youknowriad added a commit that referenced this pull request Jan 28, 2019

RichText: List: Fix indent/outdent (#12667)
* 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

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

daniloercoli added a commit that referenced this pull request Jan 28, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-move-title-to-rich-text

* 'master' of https://github.com/WordPress/gutenberg:
  Adds undo/redo support for the title. (#13514)
  Fix typo (#13524)
  RichText: List: Fix indent/outdent (#12667)
  Fixes tense on verb “assign” to correct tense (#13541)
  Plugin: Deprecate metabox supports, fall back to core (#13449)
  Rnmobile/upload media file (#13128)
  Plugin: Remove redundant core compatibility from plugin (#13442)
  Testing: Remove PHPUnit tests covering core functions (#13513)
  Plugin: Deprecate gutenberg_add_gutenberg_post_state (#13471)

@iseulde iseulde referenced this pull request Jan 29, 2019

Merged

RichText: List: Fix outdent with children #13559

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