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

Add Inline Images and Inline Blocks API #6959

Merged
merged 14 commits into from
Jun 28, 2018
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 25, 2018

Description

This PR builds on #5794. Fixes #2043.

Summary, comparison to previous PR:

  • Rebased with new inserter design.
  • Moves inline token API under the RichText component.
  • Move image specific logic to token registration.
  • Uses a portal to add items in the inserter.
  • Removes storing anything in the global application state. Everything goes through the portal.
  • Insertion point is visible on hover. This is because the inserter has items with different insertion points. Also uses admin theme colour now.
  • Inline items are visible together with block items when a RichText field has focus.
  • Updates the e2e test. Deletes the image after the test is done.

How has this been tested?

Place the caret in a RichText area. Open the inserter. "Inline image" should appear under "Inline blocks". On click the media modal opens. Select an image, and it should be inserted in the input field.

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 [Status] In Progress Tracking issues with work in progress label May 25, 2018
@ellatrix ellatrix self-assigned this May 25, 2018
@@ -67,6 +68,7 @@ export function initializeEditor( id, post, settings ) {
const reboot = reinitializeEditor.bind( null, target, settings );

registerCoreBlocks();
registerCoreInlineBlocks();
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should just move these blocks in the core-block folder, even though the API would be slightly different.

Copy link
Member

Choose a reason for hiding this comment

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

What about grouping with RichText?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I was thinking of removing this separate folder and putting it in core-blocks, so it would all be registered together. These would have inline as the category. How does that sound?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, not sure we should group these with blocks as they are different entities. To me, they are more like utilities for RichText, which is in turn a component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved some stuff around. Still polishing.

@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 12b8c87 to 3897c89 Compare May 25, 2018 18:07

category: 'inline',

edit( { onSave } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how you added edit and save functions to provide an interface similar to regular blocks. I wonder if it will be possible to have the edit toolbar in #5794 (comment) be part of this edit function as well? Not sure if this is possible though, since I think the React element returned from the save function will be rendered to string before using the edit toolbar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably will have to be some different method. We can name these differently based on what it's for. Maybe insert, edit/select, and save or similar.

/>
}
{ type &&
<type.edit onSave={ this.onSave( type ) } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice how you moved the Media Library logic out of this component by defining an edit function for inline blocks and moving the MediaUpload component to the edit function of the inline image.

@jayshenk
Copy link
Contributor

Cool how you have inline blocks working with the new Inserter design! Just noting there was previous discussion in #5794 (comment) about how at this point we may want to also show regular blocks in the menu at the same time, and there is a new mockup that is being discussed in #6998 (comment) for this. CC @jasmussen

@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch 2 times, most recently from 073a661 to 795f82b Compare May 31, 2018 13:26
@ellatrix ellatrix closed this May 31, 2018
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 795f82b to c93b778 Compare May 31, 2018 13:43
@ellatrix ellatrix reopened this May 31, 2018
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 4bb9ee5 to 8c1b8ec Compare May 31, 2018 13:54
@ellatrix
Copy link
Member Author

@jasmussen Question: if we show both inline and block items in the inserter at the same time, how doe we show insertion points?

@ellatrix
Copy link
Member Author

Should we do it again on hover as it used to be?

@ellatrix ellatrix added this to the 3.0 milestone May 31, 2018
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 8c1b8ec to 6fdfc20 Compare May 31, 2018 17:46
@aduth aduth removed this from the 3.0 milestone May 31, 2018
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch 5 times, most recently from 7527cb2 to 316104a Compare June 1, 2018 09:52
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Jun 1, 2018
@@ -54,13 +54,9 @@ class Slot extends Component {

const fills = map( getFills( name ), ( fill ) => {
const fillKey = fill.occurrence;
const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children;
Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change to allow null values.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're changing existing functionality, though, right? We're no longer injecting fillKey in "regular" cases (function-as-child).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I follow the purpose of this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

How so? There's still a key added. This change is needed because you can't return null when using the function. See https://github.com/WordPress/gutenberg/pull/6959/files#diff-8c9cd11f122f43914bc769f4ad5c36d9R21.

@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 316104a to ea92dea Compare June 1, 2018 10:12
@ellatrix ellatrix requested a review from a team June 1, 2018 10:23
@ellatrix ellatrix added the [Feature] Block API API that allows to express the block paradigm. label Jun 1, 2018
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 0fabb77 to 0054f04 Compare June 27, 2018 14:34
@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch from 102e17f to ebc5d75 Compare June 27, 2018 15:03
@ellatrix
Copy link
Member Author

@mcsf did you supply username, password, and/or base URL if it's different?

@ellatrix
Copy link
Member Author

So the problem seems to appear at await page.waitForSelector( '.media-modal li.attachment' );...

@mcsf
Copy link
Contributor

mcsf commented Jun 28, 2018

did you supply username, password, and/or base URL if it's different?

No, as my local test site is the one set up by default by our scripts.

@ellatrix
Copy link
Member Author

So my tests pass locally headless and with-head. Travis only has this error with the media modal, unsure what's causing it. The first time selecting in the media modal works, the second time it does not.

@ellatrix ellatrix force-pushed the jayshenk-add/inline-images branch 3 times, most recently from 12cf0ed to ebc5d75 Compare June 28, 2018 13:17
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Let's get this in 👍

@ellatrix ellatrix merged commit 3fd1328 into master Jun 28, 2018
@ellatrix ellatrix deleted the jayshenk-add/inline-images branch June 28, 2018 15:33
@ellatrix
Copy link
Member Author

Yay

@jasmussen
Copy link
Contributor

H O L Y G U A C A M O L E

🎉

@mtias
Copy link
Member

mtias commented Jul 5, 2018

@iseulde thanks! :)

await page.click( '.media-modal button.media-button-select' );

// Check the content.
const regex = new RegExp( '<!-- wp:paragraph -->\\s*<p>a\\u00A0<img class="wp-image-\\d+" style="width:10px" src="[^"]+\\/' + filename + '\\.png" alt="" \\/><\\/p>\\s*<!-- \\/wp:paragraph -->' );
Copy link
Member

Choose a reason for hiding this comment

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

Do we really expect the \u00A0 character here? Why not just a plain space?

@hegerpkd
Copy link

Is there a way I can de-register inline blocks? It is not apparent from the code to me. I would like to have the ability to switch off the inline images block.

I can use unregisterBlockType for blocks, but not for components and inline image.

@aduth
Copy link
Member

aduth commented Jul 31, 2018

Where is the developer documentation reference for inline blocks?

@ellatrix
Copy link
Member Author

@hegerpkd @aduth After #7890, I'd like to merge some of the current RichText extension points into one API. I'd prefer to leave current documentation as is until Gutenberg itself uses it fully for it's own RichText extension such as mentions.

@tomislavp83
Copy link

tomislavp83 commented Sep 12, 2018

@hegerpkd any updates on documentation ? I would really like to know how to register my custom inline block (token). I had some luck with registerToken function but without documentation it's useless.

@designsimply
Copy link
Member

@tomislavp83 comments on closed pull requests sometimes won't get seen easily. Would you mind opening a new issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Media Anything that impacts the experience of managing media
Projects
None yet
Development

Successfully merging this pull request may close these issues.