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

BlockList: use hooks #18821

Merged
merged 3 commits into from Dec 6, 2019
Merged

BlockList: use hooks #18821

merged 3 commits into from Dec 6, 2019

Conversation

@ellatrix
Copy link
Member

ellatrix commented Nov 29, 2019

Description

Other than that everything should be the same.
Extracts all multi block selection logic to a hook.

How has this been tested?

Screenshots

Types of changes

Refactoring

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
isDraggable,
renderAppender,
} ) {
const selector = ( select ) => {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 2, 2019

Contributor

Nit: First time I see this pattern (assigning a variable instead of just using an inline function). Not against it but there's something to be said about consistency across our components.

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Dec 2, 2019

Contributor

Agreed, I think it would make more sense to just use a function declaration.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 2, 2019

Author Member

Then the constant names will clash and I’d have to rename them.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 4, 2019

Author Member

In other words, I believe this is needed because some selectors have the same name as the constants, for example isSelectionEnabled: isSelectionEnabled().

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 4, 2019

Author Member

Assigning the constants on the next line fixes this issue.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 4, 2019

Author Member

Anything I can change here for this PR? Should I renamed the constants or leave it like this with the selector function in front of the assignments?

This comment has been minimized.

Copy link
@aduth

aduth Dec 4, 2019

Member

It seems fine to me as currently implemented. I think the bigger challenge will be to consider the impact for other use of useSelect, documenting these conventions if appropriate, etc.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 5, 2019

Member

Would it work as the following?

const result = useSelect( ( select ) => {
   //...all the logic goes here
} );
const { x, y, z } = result;

I don't know how the React linter for hooks work, but we should ensure that it catches the case when props are used in useSelect and they aren't passed as 2nd param:

const result = useSelect( ( select ) => {
   //...all the logic goes here
   return {
       x: select( 'store' ).methodA( props.a ),
       y: select( 'store' ).methodB( props.b ),
       z: select( 'store' ).methodC( props.c ),
   };
}, [ props.a, props.b, props.c ] );
const { x, y, z } = result;

Well, it's probably something that we don't support at the moment anyway based on the code snippets I found. Anyway, I'm just sharing as something which might make the difference but doesn't have to :)

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

That works too. I don't really mind either way. What shall we do?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

Let's leave it as it is and change these lines if we decide to do it differently?

// `onSelectionStart` is called after `mousedown` and `mouseleave`
// (from a block). The selection ends when `mouseup` happens anywhere
// in the window.
window.addEventListener( 'mouseup', onSelectionEnd );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 2, 2019

Contributor

I think personally having this listened and its cleanup in the same effect is better in terms of clarity. Do you expect any noticeable impact on performance? The onSelectionEnd could check whether a multi-selection is in progress.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 4, 2019

Author Member

It will depend on the type of content. If it's just a list of paragraphs, then no. If the content has many different block lists, like full site editing with complex content, then maybe? I think we should prevent any unnecessary listeners where possible. I don't think we should not do it because it's an uncommon pattern. If it's unclear, I should comment better. :)

@ellatrix ellatrix force-pushed the try/block-list-hooks branch from 03ec6c2 to a6bf2c1 Dec 4, 2019
@ellatrix ellatrix requested review from youknowriad and ZebulanStanphill Dec 4, 2019
@ellatrix ellatrix force-pushed the try/block-list-hooks branch from a20a30e to aa4d84d Dec 4, 2019
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 6, 2019

I haven't been able to find any obvious errors in the code, but it's hard to test the branch for regressions right now since it needs rebasing and there are some various selection/focus issues that are/were present in master. I think #18943 needs to be merged and then this PR should be rebased.

@ellatrix ellatrix force-pushed the try/block-list-hooks branch from a061abb to 671bf41 Dec 6, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 6, 2019

@youknowriad Curious what you think about this extraction to a hook. :)

enableAnimation,
} = useSelect( selector );
const ref = useRef();
const onSelectionStart = useSelection( { ref, rootClientId } );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 6, 2019

Contributor

I like the extraction :) I believe it should "mutliSelection" and not just "selection" to avoid confusion.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 6, 2019

Contributor

I think it would be good to check performance impact (new useSelect...)

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

Done :)

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

Oh, right. I'll run the tests.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

Seems to be more or less the same.

This branch:

Average time to load: 4447ms
Average time to DOM content load: 4163ms
Average time to type character: 68.645ms
Slowest time to type character: 116ms
Fastest time to type character: 56ms

master:

Average time to load: 4510ms
Average time to DOM content load: 4224ms
Average time to type character: 69.04ms
Slowest time to type character: 118ms
Fastest time to type character: 50ms
*/
const onSelectionEnd = useCallback( () => {
document.removeEventListener( 'selectionchange', onSelectionChange );
// Equivalent to attaching the listener once.

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Dec 6, 2019

Contributor

This comment is confusing to me. I'm not sure what exactly it is trying to say.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 6, 2019

Author Member

Perhaps it's not really necessary to comment. I see the mouseup listener as equivalent to

window.addEventListener( 'mouseup', listener, { once: true } );

because here we remove the listener on the first call.

Copy link
Contributor

ZebulanStanphill left a comment

I can't find any obvious errors in the code, and everything appears to be working the same as master, so approving.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 6, 2019

Thanks @ZebulanStanphill!

@ellatrix ellatrix merged commit 52d2a87 into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the try/block-list-hooks branch Dec 6, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.