Skip to content

Fix line break insertion #10297

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
Merged
ellatrix merged 5 commits into from
Oct 4, 2018
Merged

Fix line break insertion #10297

ellatrix merged 5 commits into from
Oct 4, 2018

Conversation

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 3, 2018 •
edited
Loading

Description

Fixes an issue where inserting like breaks would throw an error. This either happens when pressing shift+enter in a rich text field that can be split, like a paragraph, or when pressing enter in a rich text field that cannot be split, like a caption.

  1. It replaces the TinyMCE command to insert a line break with rich text value helpers.
  2. "to dom" functionality is update to handle selection on br elements.
  3. "create" tests are now reused for "to dom", to ensure the other way around has enough coverage.
  4. A few e2e tests are added.

How has this been tested?

Press enter in e.g. a caption. A line break should be added.

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 ellatrix changed the title Fix line line break insertion Fix line break insertion Oct 3, 2018
@ellatrix ellatrix added this to the 4.0 milestone Oct 3, 2018
@ellatrix ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 3, 2018
@ellatrix ellatrix requested a review from a team October 3, 2018 12:45
@ellatrix ellatrix mentioned this pull request Oct 4, 2018
Closed
4 tasks
@ellatrix ellatrix added the [Priority] High Used to indicate top priority items that need quick attention label Oct 4, 2018
@ellatrix ellatrix self-assigned this Oct 4, 2018
mcsf
mcsf approved these changes Oct 4, 2018
View reviewed changes
Copy link
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.

Raised some questions, but aside from those it looks good to me.

Just noting that I get deprecation notices in Image, though:

screenshot 2018-10-04 at 11 57 34

test/e2e/specs/writing-flow.test.js Outdated
await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.down( 'Shift' );
await page.keyboard.press( 'Enter' );
await page.keyboard.up( 'Shift' );
Copy link
Contributor

@mcsf mcsf Oct 4, 2018

Choose a reason for hiding this comment

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

We have a very handy helper: pressWithModifier( 'Shift', 'Enter' )

Copy link
Member Author

@ellatrix ellatrix Oct 4, 2018

Choose a reason for hiding this comment

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

This doesn't seem to work for me.

Copy link
Member Author

@ellatrix ellatrix Oct 4, 2018

Choose a reason for hiding this comment

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

Oh never mind :)

packages/editor/src/components/rich-text/index.js Outdated
Copy link
Contributor

@mcsf mcsf Oct 4, 2018

Choose a reason for hiding this comment

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

Why Unicode 2028 instead of a plain (ASCII-level) line break?

Copy link
Member Author

@ellatrix ellatrix Oct 4, 2018

Choose a reason for hiding this comment

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

Because we're using that to delimit lines in multiline values. We could probably benefit from a helper or a constant...

Copy link
Member Author

@ellatrix ellatrix Oct 4, 2018

Choose a reason for hiding this comment

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

And \n are used for single line breaks which convert to <br> elements.

Copy link
Contributor

@mcsf mcsf Oct 4, 2018

Choose a reason for hiding this comment

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

That's fair, but let's add a comment for it.

Copy link
Contributor

@mcsf mcsf Oct 4, 2018

Choose a reason for hiding this comment

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

Comment then 🚢 🚢 !

@ellatrix
Copy link
Member Author

ellatrix commented Oct 4, 2018

Just noting that I get deprecation notices in Image, though:

Yes, noticed this with Jorge the other day, will fix separately.

@ellatrix ellatrix merged commit fb135fb into master Oct 4, 2018
@ellatrix ellatrix deleted the fix/rich-text-br branch October 4, 2018 18:35
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

Assignees

@ellatrix ellatrix

Labels

[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Priority] High Used to indicate top priority items that need quick attention

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ellatrix @mcsf