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

Import Format API components instead of passing as props #11545

Merged
merged 4 commits into from
Nov 6, 2018
Merged

Import Format API components instead of passing as props #11545

merged 4 commits into from
Nov 6, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 6, 2018

Description

Refactoring. See #11488 (comment).

How has this been tested?

Ensure toolbar buttons work, shortcuts work, an the inline image button works. Also ensure that searching for the inline image button works in the inserter.

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 Nov 6, 2018
@ellatrix ellatrix added this to the 4.2 milestone Nov 6, 2018
@ellatrix ellatrix requested review from youknowriad, gziolo and a team November 6, 2018 14:18
@gziolo
Copy link
Member

gziolo commented Nov 6, 2018

Cool, thanks for working on it. I quickly checked the code and it looks exactly as I would expect. I need to take a closer look before I give 👍

@ellatrix
Copy link
Member Author

ellatrix commented Nov 6, 2018

On thing I'm not sure about is naming.

@mtias
Copy link
Member

mtias commented Nov 6, 2018

The naming of the components?

@ellatrix
Copy link
Member Author

ellatrix commented Nov 6, 2018

Yeah, I just prefixed them all with RichText. I guess that's fine. :)

@gziolo
Copy link
Member

gziolo commented Nov 6, 2018

Can we update RichTextInserterListItem to RichTextInserterInlineItem?

@ellatrix
Copy link
Member Author

ellatrix commented Nov 6, 2018

Why not just RichTextInserterItem, as any rich text item in there will be automatically inline?

gziolo
gziolo approved these changes Nov 6, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome, tests well. Code looks good, too 👍

Why not just RichTextInserterItem, as any rich text item in there will be automatically inline?

Yes, true. It's also shorter.

@ellatrix ellatrix merged commit 9195550 into master Nov 6, 2018
@youknowriad youknowriad deleted the try/format-api-import-components branch November 6, 2018 15:56
youknowriad pushed a commit that referenced this pull request Nov 6, 2018
@ellatrix @youknowriad
Import Format API components instead of passing as props (#11545)
5338625 
* Import Format API components instead of passing as props

* Include button title in inserter search

* Simplify isResult

* RichTextInserterListItem => RichTextInserterItem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@ellatrix @gziolo @mtias