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

Blocks: (Re-)enable selection of embed block by clicking #5730

Merged
merged 8 commits into from
Apr 13, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 21, 2018

Closes #5589
Related: #483

This pull request seeks to restore behaviors of embed click-to-select, originally implemented in #4011 and later regressing in #4872. In doing so, it adds a new function property passed to a block to allow a developer to programmatically select or deselect the block. This, in combination with a new FocusableIframe component, is leveraged to allow the embed block to be selected when its iframe receives focus.

Testing instructions:

Repeat testing instructions from #4011

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Mar 21, 2018
@aduth aduth force-pushed the add/focusable-iframe branch from a095e52 to fa89a1d Compare April 11, 2018 19:32
@aduth aduth requested a review from a team April 11, 2018 19:32
@aduth
Copy link
Member Author

aduth commented Apr 11, 2018

Rebased to resolve conflicts, and to refactor some of the components and higher-order components to more recent patterns, including using createHigherOrderComponent for withGlobalEvents, but more interestingly React 16.3.0's new createRef API. It can serve as an example for how to avoid findDOMNode even in the cases where we'd think we need it, using a documented technique for passing a created ref as a prop to a component.

On behalf of the consuming developer, the higher-order component manages:

- Unbinding when the component unmounts.
- Binding at most a single event handler for the entire application.
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 this component, especially this part 👍 Nice job


this.state = {
width: 0,
height: 0,
};
}

componentDidMount() {
window.addEventListener( 'message', this.checkMessageForResize, false );
Copy link
Contributor

Choose a reason for hiding this comment

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

withGlobalEvent?

Copy link
Member Author

Choose a reason for hiding this comment

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

withGlobalEvent?

Hah! But in seriousness, yes, it should! Will add.


## Usage

Use as you would a standard `iframe`, passing `onFocus` as the callback to be invoked when the iframe receives focus.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess iframeRef could be documented as well?

@@ -96,6 +99,10 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k
this.unmounting = true;
}

setIsSelected() {
this.props.setIsSelected( true );
Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing bothering me with this PR is this prop, I mean, if we could avoid it by bubbling the focus event or something, it would have been great. But I understand it might be tricky.

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 mean, if we could avoid it by bubbling the focus event or something, it would have been great.

That would be interesting. Let me play with it a bit to see if I can get it to work.

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 mean, if we could avoid it by bubbling the focus event or something, it would have been great.

That would be interesting. Let me play with it a bit to see if I can get it to work.

Luckily it just works to dispatch a custom event. See 8a6c493. Simplifies things a fair bit.

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.

Nice work on this PR 👍

resize: 'handleResize',
} )( OriginalComponent );

wrapper = mount( <EnhancedComponent { ...props }>Hello</EnhancedComponent> );
Copy link
Member

Choose a reason for hiding this comment

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

I started to think if we should introduce our own mount or further enrich enzyme to call unmount behind the scenes when the test finishes.

Copy link
Member

Choose a reason for hiding this comment

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

let wrapper;
export const mount = ( ...params ) => {
	wrapper = originalMount( ...params );
	return wrapper;
};

afterEach( () => {
	if ( wrapper ) {
		wrapper.unmount();
		wrapper = null;
	}
} );

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 started to think if we should introduce our own mount or further enrich enzyme to call unmount behind the scenes when the test finishes.

I'd like this 👍

Copy link
Member

Choose a reason for hiding this comment

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

I will check it next week. jest.mount( element ) or jest.shallow( element ) might also save some keystrokes :)


listener.handleEvent( event );

expect( handler.handleEvent ).toHaveBeenCalledWith( event );
Copy link
Member

Choose a reason for hiding this comment

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

If I would add the same listener 5 times:

listener.add( 'resize', handler );
listener.add( 'resize', handler );
listener.add( 'resize', handler );
listener.add( 'resize', handler );
listener.add( 'resize', handler );

would that be true:

expect( handler.handleEvent ).toHaveBeenCalledTimes( 5 );

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I confirmed it in my tests 👍

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to remove c41e84d if you find it useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to remove c41e84d if you find it useless.

Definitely useful. Thanks for the addition.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Yeah, excellent work.

*
* @return {Object} Ref object.
*/
export { createRef };
Copy link
Member

Choose a reason for hiding this comment

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

We still need to add forwardRef - it can be done in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need to add forwardRef - it can be done in another PR.

Yeah, I suppose we'd want to build this into createHigherOrderComponent ?

Copy link
Member

Choose a reason for hiding this comment

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

RichText with Autocomplete has a few wrappers with refs, so we can check there. I guess HOC makes the most sense in our case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants