Skip to content

RichText: List: fix indentation#13563

Merged
ellatrix merged 4 commits intomasterfrom
fix/list-indent
Jan 29, 2019
Merged

RichText: List: fix indentation#13563
ellatrix merged 4 commits intomasterfrom
fix/list-indent

Conversation

@ellatrix
Copy link
Copy Markdown
Member

@ellatrix ellatrix commented Jan 29, 2019 •
edited
Loading

Description

Fixes #13559 (comment). We should check the previous list item's direct children, not the trailing list item, to copy formats from.

How has this been tested?

See #13559 (comment).

Screenshots

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix requested review from a team, mcsf and youknowriad January 29, 2019 13:56
@ellatrix ellatrix added the [Package] Rich text /packages/rich-text label Jan 29, 2019
@ellatrix ellatrix added this to the 5.0 (Gutenberg) milestone Jan 29, 2019
@ellatrix ellatrix changed the title fix list indentation RichText: List: fix indentation Jan 29, 2019
@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Jan 29, 2019

I can't tell if with all the branches merged together this will still happen, but here's what I'm getting:

indent-outdent

(The screencast tool, Licecap, doesn't show click events, but trust that I'm clicking. 😉)

  1. I'm unable to outdent D all the way back.
  2. When I switch to B, outdenting it also brings D up a level.

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 29, 2019

@mcsf The other PR fixes the outdent issue. This one is for the indent issue. See #13562.

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 29, 2019

When I switch to B, outdenting it also brings D up a level.

I don't know what's the expected behaviour here. The whole list is outdented, so that's correct?

@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Jan 29, 2019

When I switch to B, outdenting it also brings D up a level.

I don't know what's the expected behaviour here. The whole list is outdented, so that's correct?

Actually, yeah, scratch that. I wasn't thinking properly. :)

@mcsf The other PR fixes the outdent issue. This one is for the indent issue. See #13562.

Yup. I suggest merging one of them, perhaps the one you feel the most confident about, rebasing the other and making sure that everything aligns well.

@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Jan 29, 2019

  1. Starting with
<!-- wp:list -->
<ul><li>A</li><li>B<ul><li>C</li></ul></li><li>D</li></ul>
<!-- /wp:list -->
  1. Select both C and D.
  2. Outdent.

Expected: C returns to level 0, D stays at 0.
Observed: nothing changes, and a TypeError is thrown:

TypeError: undefined is not an object (evaluating 'newFormats[index].slice')

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 29, 2019

@mcsf Ok, this is again outdenting. I'll just fix it here.

@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Jan 29, 2019

@mcsf Ok, this is again outdenting. I'll just fix it here.

I though you had rebased the branch on top of #13559? That's why I thought it was pertinent feedback. If not, carry on.

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 29, 2019

I mean rather than go with "it's already an issue in master, let's fix separately", I said, let's just fix it in this PR :) It's fixed now

mcsf
mcsf approved these changes Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Great work!

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Jan 29, 2019

Yay, thanks for the reviews @mcsf!

@ellatrix ellatrix merged commit d9cb84e into master Jan 29, 2019
@ellatrix ellatrix deleted the fix/list-indent branch January 29, 2019 17:04
youknowriad pushed a commit that referenced this pull request Jan 29, 2019
@ellatrix @youknowriad
RichText: List: fix indentation (#13563)
b4c68a5 
* fix list indentation

* Add more tests

* Guard against negative lineIndex

* Fix outdent error
daniloercoli added a commit that referenced this pull request Jan 30, 2019
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
68b353c 
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (36 commits)
  Fixes plural messages POT generation. (#13577)
  Typo fix (#13595)
  REST API: Remove oEmbed proxy HTML filtering (#13575)
  Removed unnecessary className attribute. Fixes #11664 (#11831)
  Add changelog for RSS block (#13588)
  Components: Set type=button for TabPanel button elements. (#11944)
  Update util.js (#13582)
  Docs: Add accessbility specific page (#13169)
  Rnmobile/media methods refactor (#13554)
  chore(release): publish
  chore(release): publish
  Plugin: Deprecate gutenberg_get_script_polyfill (#13536)
  Block API: Parse entity only when valid character reference (#13512)
  RichText: List: fix indentation (#13563)
  Plugin: Deprecate window._wpLoadGutenbergEditor (#13547)
  Plugin: Avoid setting generic "Edit Post" title on load (#13552)
  Plugin: Populate demo content by default content filters (#13553)
  RichText: List: Fix getParentIndex (#13562)
  RichText: List: Fix outdent with children (#13559)
  Scripts: Remove npm run build from test-e2e default run (#13420)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcsf mcsf mcsf approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

No one assigned

Labels

[Package] Rich text /packages/rich-text

Projects

None yet

Milestone

5.0 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

2 participants

@ellatrix @mcsf