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.

Sign up for GitHub

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

Jump to bottom

RichText: List: Fix indent/outdent #12667

Merged
merged 15 commits into from
Jan 28, 2019
Merged

RichText: List: Fix indent/outdent #12667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2fff02f
RichText: List: use value to indent/outdent
ellatrix Dec 6, 2018
fffaa50
Add outdent
ellatrix Dec 6, 2018
7d74d99
Support multi outdent
ellatrix Dec 6, 2018
42f9d86
Hold one reference per format
ellatrix Dec 6, 2018
72e7c15
Keep list formats when indenting to new index
ellatrix Dec 6, 2018
e768938
Remove unneeded parameter
ellatrix Dec 6, 2018
c55934a
Rename
ellatrix Dec 6, 2018
13139f3
Add logic to change list type
ellatrix Dec 6, 2018
699e6d5
Add tests
ellatrix Jan 11, 2019
b84f0b9
Add e2e tests
ellatrix Jan 15, 2019
b94e41d
Add some basic docs. Clean up.
ellatrix Jan 24, 2019
0ab1e63
Remove duplicate wp-tinymce dependency
ellatrix Jan 24, 2019
6c77f19
Clean up
ellatrix Jan 25, 2019
a205d8f
Add more docs, fix getSelectedListNode
ellatrix Jan 25, 2019
e4dc45d
Put duplicate text values in constant
ellatrix Jan 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/packages-dependencies.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@
),
'wp-editor' => array(
'lodash',
'wp-tinymce-lists',
'wp-a11y',
'wp-api-fetch',
'wp-blob',
Expand Down
42 changes: 42 additions & 0 deletions packages/e2e-tests/specs/blocks/__snapshots__/list.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ exports[`List should be immeadiately saved on indentation 1`] = `
<!-- /wp:list -->"
`;

exports[`List should change the base list type 1`] = `
"<!-- wp:list {\\"ordered\\":true} -->
<ol><li></li></ol>
<!-- /wp:list -->"
`;

exports[`List should change the indented list type 1`] = `
"<!-- wp:list -->
<ul><li>a<ol><li>1</li></ol></li></ul>
<!-- /wp:list -->"
`;

exports[`List should create paragraph on split at end and merge back with content 1`] = `
"<!-- wp:list -->
<ul><li>one</li></ul>
Expand All @@ -102,6 +114,36 @@ exports[`List should create paragraph on split at end and merge back with conten
<!-- /wp:list -->"
`;

exports[`List should indent and outdent level 1 1`] = `
"<!-- wp:list -->
<ul><li>a<ul><li>1</li></ul></li></ul>
<!-- /wp:list -->"
`;

exports[`List should indent and outdent level 1 2`] = `
"<!-- wp:list -->
<ul><li>a</li><li>1</li></ul>
<!-- /wp:list -->"
`;

exports[`List should indent and outdent level 2 1`] = `
"<!-- wp:list -->
<ul><li>a<ul><li>1<ul><li>i</li></ul></li></ul></li></ul>
<!-- /wp:list -->"
`;

exports[`List should indent and outdent level 2 2`] = `
"<!-- wp:list -->
<ul><li>a<ul><li>1</li><li>i</li></ul></li></ul>
<!-- /wp:list -->"
`;

exports[`List should indent and outdent level 2 3`] = `
"<!-- wp:list -->
<ul><li>a<ul><li>1</li></ul></li><li>i</li></ul>
<!-- /wp:list -->"
`;

exports[`List should split indented list item 1`] = `
"<!-- wp:list -->
<ul><li>one<ul><li>two</li><li>three</li></ul></li></ul>
Expand Down
59 changes: 59 additions & 0 deletions packages/e2e-tests/specs/blocks/list.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,63 @@ describe( 'List', () => {

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should change the base list type', async () => {
await insertBlock( 'List' );
await page.click( 'button[aria-label="Convert to ordered list"]' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should change the indented list type', async () => {
await insertBlock( 'List' );
await page.keyboard.type( 'a' );
await page.keyboard.press( 'Enter' );
await pressKeyWithModifier( 'primary', 'm' );
await page.keyboard.type( '1' );

// Pointer device is needed. Shift+Tab won't focus the toolbar.
// To do: fix so Shift+Tab works.
await page.mouse.move( 200, 300, { steps: 10 } );
await page.mouse.move( 250, 350, { steps: 10 } );

await page.click( 'button[aria-label="Convert to ordered list"]' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should indent and outdent level 1', async () => {
await insertBlock( 'List' );
await page.keyboard.type( 'a' );
await page.keyboard.press( 'Enter' );
await pressKeyWithModifier( 'primary', 'm' );
await page.keyboard.type( '1' );

expect( await getEditedPostContent() ).toMatchSnapshot();

await pressKeyWithModifier( 'primaryShift', 'm' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should indent and outdent level 2', async () => {
await insertBlock( 'List' );
await page.keyboard.type( 'a' );
await page.keyboard.press( 'Enter' );
await pressKeyWithModifier( 'primary', 'm' );
await page.keyboard.type( '1' );
await page.keyboard.press( 'Enter' );
await pressKeyWithModifier( 'primary', 'm' );
await page.keyboard.type( 'i' );

expect( await getEditedPostContent() ).toMatchSnapshot();

await pressKeyWithModifier( 'primaryShift', 'm' );

expect( await getEditedPostContent() ).toMatchSnapshot();

await pressKeyWithModifier( 'primaryShift', 'm' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );
17 changes: 3 additions & 14 deletions packages/editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ export class RichText extends Component {
this.onSplit = this.props.unstableOnSplit;
}

this.onSetup = this.onSetup.bind( this );
this.onFocus = this.onFocus.bind( this );
this.onBlur = this.onBlur.bind( this );
this.onChange = this.onChange.bind( this );
Expand Down Expand Up @@ -137,15 +136,6 @@ export class RichText extends Component {
this.editableRef = node;
}

/**
* Sets a reference to the TinyMCE editor instance.
*
* @param {Editor} editor The editor instance as passed by TinyMCE.
*/
onSetup( editor ) {
this.editor = editor;
}

setFocusedElement() {
if ( this.props.setFocusedElement ) {
this.props.setFocusedElement( this.props.instanceId );
Expand Down Expand Up @@ -837,12 +827,12 @@ export class RichText extends Component {
<div className={ classes }
onFocus={ this.setFocusedElement }
>
{ isSelected && this.editor && this.multilineTag === 'li' && (
{ isSelected && this.multilineTag === 'li' && (
<ListEdit
editor={ this.editor }
onTagNameChange={ onTagNameChange }
tagName={ Tagname }
onSyncDOM={ () => this.onChange( this.createRecord() ) }
value={ record }
onChange={ this.onChange }
/>
) }
{ isSelected && ! inlineToolbar && (
Expand All @@ -865,7 +855,6 @@ export class RichText extends Component {
<Fragment>
<TinyMCE
tagName={ Tagname }
onSetup={ this.onSetup }
style={ style }
record={ record }
valueToEditableHTML={ this.valueToEditableHTML }
Expand Down
122 changes: 84 additions & 38 deletions packages/editor/src/components/rich-text/list-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
import { Toolbar } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { Fragment } from '@wordpress/element';
import {
indentListItems,
outdentListItems,
changeListType,
} from '@wordpress/rich-text';

/**
* Internal dependencies
Expand All @@ -13,60 +18,105 @@ import { Fragment } from '@wordpress/element';
import { RichTextShortcut } from './shortcut';
import BlockFormatControls from '../block-format-controls';

function isListRootSelected( editor ) {
return (
! editor.selection ||
editor.selection.getNode().closest( 'ol,ul' ) === editor.getBody()
);
}
const { TEXT_NODE, ELEMENT_NODE } = window.Node;

function isActiveListType( editor, tagName, rootTagName ) {
if ( document.activeElement !== editor.getBody() ) {
return tagName === rootTagName;
/**
* Gets the selected list node, which is the closest list node to the start of
* the selection.
*
* @return {?Element} The selected list node, or undefined if none is selected.
*/
function getSelectedListNode() {
const selection = window.getSelection();

if ( selection.rangeCount === 0 ) {
return;
}

const listItem = editor.selection.getNode();
const list = listItem.closest( 'ol,ul' );
let { startContainer } = selection.getRangeAt( 0 );

if ( startContainer.nodeType === TEXT_NODE ) {
startContainer = startContainer.parentNode;
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
}

if ( ! list ) {
if ( startContainer.nodeType !== ELEMENT_NODE ) {
return;
}

return list.nodeName.toLowerCase() === tagName;
const rootNode = startContainer.closest( '*[contenteditable]' );

if ( ! rootNode || ! rootNode.contains( startContainer ) ) {
return;
}

return startContainer.closest( 'ol,ul' );
}

export const ListEdit = ( { editor, onTagNameChange, tagName, onSyncDOM } ) => (
/**
* Whether or not the root list is selected.
*
* @return {boolean} True if the root list or nothing is selected, false if an
* inner list is selected.
*/
function isListRootSelected() {
const listNode = getSelectedListNode();

// Consider the root list selected if nothing is selected.
return ! listNode || listNode.contentEditable === 'true';
}

/**
* Wether or not the selected list has the given tag name.
*
* @param {string} tagName The tag name the list should have.
* @param {string} rootTagName The current root tag name, to compare with in
* case nothing is selected.
*
* @return {boolean} [description]
*/
function isActiveListType( tagName, rootTagName ) {
const listNode = getSelectedListNode();

if ( ! listNode ) {
return tagName === rootTagName;
}

return listNode.nodeName.toLowerCase() === tagName;
}

export const ListEdit = ( {
onTagNameChange,
tagName,
value,
onChange,
} ) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

<Fragment>
<RichTextShortcut
type="primary"
character="["
onUse={ () => {
editor.execCommand( 'Outdent' );
onSyncDOM();
onChange( outdentListItems( value ) );
} }
/>
<RichTextShortcut
type="primary"
character="]"
onUse={ () => {
editor.execCommand( 'Indent' );
onSyncDOM();
onChange( indentListItems( value, { type: tagName } ) );
} }
/>
<RichTextShortcut
type="primary"
character="m"
onUse={ () => {
editor.execCommand( 'Indent' );
onSyncDOM();
onChange( indentListItems( value, { type: tagName } ) );
} }
/>
<RichTextShortcut
type="primaryShift"
character="m"
onUse={ () => {
editor.execCommand( 'Outdent' );
onSyncDOM();
onChange( outdentListItems( value ) );
} }
/>
<BlockFormatControls>
Expand All @@ -75,43 +125,39 @@ export const ListEdit = ( { editor, onTagNameChange, tagName, onSyncDOM } ) => (
onTagNameChange && {
icon: 'editor-ul',
title: __( 'Convert to unordered list' ),
isActive: isActiveListType( editor, 'ul', tagName ),
isActive: isActiveListType( 'ul', tagName ),
onClick() {
if ( isListRootSelected( editor ) ) {
onChange( changeListType( value, { type: 'ul' } ) );

if ( isListRootSelected() ) {
onTagNameChange( 'ul' );
} else {
editor.execCommand( 'InsertUnorderedList' );
onSyncDOM();
}
},
},
onTagNameChange && {
icon: 'editor-ol',
title: __( 'Convert to ordered list' ),
isActive: isActiveListType( editor, 'ol', tagName ),
isActive: isActiveListType( 'ol', tagName ),
onClick() {
if ( isListRootSelected( editor ) ) {
onChange( changeListType( value, { type: 'ol' } ) );

if ( isListRootSelected() ) {
onTagNameChange( 'ol' );
} else {
editor.execCommand( 'InsertOrderedList' );
onSyncDOM();
}
},
},
{
icon: 'editor-outdent',
title: __( 'Outdent list item' ),
onClick() {
editor.execCommand( 'Outdent' );
onSyncDOM();
onClick: () => {
onChange( outdentListItems( value ) );
},
},
{
icon: 'editor-indent',
title: __( 'Indent list item' ),
onClick() {
editor.execCommand( 'Indent' );
onSyncDOM();
onClick: () => {
onChange( indentListItems( value, { type: tagName } ) );
},
},
].filter( Boolean ) }
Expand Down
5 changes: 0 additions & 5 deletions packages/editor/src/components/rich-text/tinymce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,16 +200,11 @@ export default class TinyMCE extends Component {
lists_indent_on_tab: false,
};

if ( multilineTag === 'li' ) {
settings.plugins.push( 'lists' );
}

tinymce.init( {
...settings,
target: this.editorNode,
setup: ( editor ) => {
this.editor = editor;
this.props.onSetup( editor );

// TinyMCE resets the element content on initialization, even
// when it's already identical to what exists currently. This
Expand Down
Loading