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.

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: absorb internal list controls #10744

Merged
merged 8 commits into from
Nov 14, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 18, 2018

Description

This change aims to absorb any RichText related logic from the list block into the RichText component. With this, none of our core blocks will use the unstableGetSettings or unstableOnSetup props, which I would like to remove entirely so that no TinyMCE APIs remain exposed.

Also fixes a bug where the the active button is not set for nested lists.

After #10799, the list buttons may be rewritten to manipulate the internal rich text value directly, removing the dependency on the TinyMCE lists plugin.

How has this been tested?

The list block should continue to work as before.

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 added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 18, 2018
@ellatrix ellatrix added this to the 4.2 - API freeze milestone Oct 18, 2018
@ellatrix ellatrix requested a review from a team October 18, 2018 15:11
@aduth
Copy link
Member

aduth commented Oct 18, 2018

With this, none of our core blocks will use the unstableGetSettings or unstableOnSetup props, which I would like to remove entirely so that no TinyMCE APIs remain exposed.

Can we delete them here and now?

@ellatrix
Copy link
Member Author

Can we delete them here and now?

Sure thing! More than happy to!

@ellatrix
Copy link
Member Author

There you go!

@ellatrix ellatrix force-pushed the try/rich-text-absorb-internal-list-controls branch from d389235 to 2af3985 Compare October 18, 2018 17:05
@ellatrix ellatrix self-assigned this Oct 18, 2018
@gziolo gziolo self-requested a review October 25, 2018 08:20
@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018
@gziolo gziolo modified the milestones: WordPress 5.0, 4.4 Nov 5, 2018
@ellatrix ellatrix modified the milestones: 4.4, 4.3 Nov 6, 2018
@ellatrix ellatrix force-pushed the try/rich-text-absorb-internal-list-controls branch 2 times, most recently from 5d15b6f to ad64fcc Compare November 6, 2018 19:10
aduth
aduth previously requested changes Nov 8, 2018
/>
<RichTextShortcut
type="primary"
character="["
Copy link
Member

Choose a reason for hiding this comment

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

These were ported backwards. ] should be for the indent.

Copy link
Member

Choose a reason for hiding this comment

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

These were ported backwards. ] should be for the indent.

Updated in 25997a2

@aduth aduth force-pushed the try/rich-text-absorb-internal-list-controls branch from 3d21666 to bacbbdf Compare November 9, 2018 13:12
@aduth
Copy link
Member

aduth commented Nov 9, 2018

There's something not working quite right with indentation on new list items. I'll plan to investigate.

@aduth
Copy link
Member

aduth commented Nov 9, 2018

It's not immediately obvious to me what's going on with the list indentation. The callbacks for the key handlers are not being called. Doing anything after the new item (even moving mouse, leaving screen and returning, pressing Escape) is enough to cause the keybind to work again, which itself is concerning as these don't seem like they should be causing any change in state to the RichText element.

We also still didn't quite port the existing binds as they were before, because Cmd+M / Cmd+Shift+M were only bound previously if the keyboard didn't support the ].

Given this is more a refactoring, and while I'd like to see the unstable functions gone sooner than later, I'm inclined to punt this to 4.4.

@aduth aduth modified the milestones: 4.3, 4.4 Nov 9, 2018
@ellatrix
Copy link
Member Author

@aduth Could you provide some steps to reproduce the indentation issue? I don't find any issues.

@ellatrix
Copy link
Member Author

Tbh the shortcuts added for lists are not great. On a Mac, 3/4 of the shortcuts clash:

screen shot 2018-11-14 at 12 37 43

screen shot 2018-11-14 at 12 38 39

@ellatrix
Copy link
Member Author

@aduth Okay, I found the problem with the shortcuts. They unmount when the toolbar disappears. So when when you type, they no longer work, when you hover again so the toolbars show, they work again.

@ellatrix ellatrix force-pushed the try/rich-text-absorb-internal-list-controls branch from bacbbdf to 7d08d2e Compare November 14, 2018 11:53
@ellatrix ellatrix dismissed aduth’s stale review November 14, 2018 11:56

Feedback addressed.

@ellatrix
Copy link
Member Author

We also still didn't quite port the existing binds as they were before, because Cmd+M / Cmd+Shift+M were only bound previously if the keyboard didn't support the ].

It's a bit strange to me that we're only binding based on some random language detection. The [] shortcuts won't do anything if you don't have the keys on your keyboard so it doesn't hurt to bind them. I don't think we can accurately detect if a keyboard has these keys

I also think we should bind Cmd+M / Cmd+Shift+M for everyone. Why would we only make them work in certain cases?

@ellatrix ellatrix requested a review from a team November 14, 2018 12:00
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@ellatrix
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants