Fill quick inserter results with available variations if limit is set and items are less. #24481

ntsekouras · 2020-08-11T12:18:43Z

Description

There was a refactoring (#24090) in embed block that made it a single block with lots of block variations. That surfaced a problem with quick Inserter and the main Inserter when suggesting items with a limit (currently six).

A small fix was there to handle the cases where the limit was set and the items where NOT fewer than the limit.

I saw now that when you're searching for items and the filtered results are for example four, it continues to add all the available variations.

This PR handles this case ( limit is set and bigger than items ) and fills the Block list with available variations, if they exist, until it reaches the provided limit.

The addition now happens by taking account only the order of variations and there is not a sophisticated handling of which variations to choose. It could/should be handled better when we decide how to handle block variations better.

Steps to reproduce

Go to edit a post/page
Clock on the quick Inserter ( plus button )
Type in search em
You can see that the results are way more than the current limit ( 6 )

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.

github-actions · 2020-08-11T12:23:56Z

Size Change: +134 B (0%)

Total Size: 1.16 MB

Filename	Size	Change
`build/block-editor/index.js`	125 kB	+123 B (0%)
`build/block-editor/style-rtl.css`	10.6 kB	-13 B (0%)
`build/block-editor/style.css`	10.6 kB	-13 B (0%)
`build/block-library/index.js`	132 kB	+4 B (0%)
`build/block-library/style-rtl.css`	7.49 kB	-9 B (0%)
`build/block-library/style.css`	7.49 kB	-9 B (0%)
`build/components/index.js`	200 kB	+13 B (0%)
`build/edit-post/style-rtl.css`	5.63 kB	+19 B (0%)
`build/edit-post/style.css`	5.63 kB	+19 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.67 kB	0 B
`build/api-fetch/index.js`	3.44 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	7.97 kB	0 B
`build/block-directory/style-rtl.css`	953 B	0 B
`build/block-directory/style.css`	952 B	0 B
`build/block-library/editor-rtl.css`	8.36 kB	0 B
`build/block-library/editor.css`	8.36 kB	0 B
`build/block-library/theme-rtl.css`	729 B	0 B
`build/block-library/theme.css`	730 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	48.4 kB	0 B
`build/components/style-rtl.css`	15.7 kB	0 B
`build/components/style.css`	15.7 kB	0 B
`build/compose/index.js`	9.68 kB	0 B
`build/core-data/index.js`	11.8 kB	0 B
`build/data-controls/index.js`	1.29 kB	0 B
`build/data/index.js`	8.45 kB	0 B
`build/date/index.js`	5.38 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	568 B	0 B
`build/dom/index.js`	3.23 kB	0 B
`build/edit-navigation/index.js`	10.9 kB	0 B
`build/edit-navigation/style-rtl.css`	1.08 kB	0 B
`build/edit-navigation/style.css`	1.08 kB	0 B
`build/edit-post/index.js`	304 kB	0 B
`build/edit-site/index.js`	17 kB	0 B
`build/edit-site/style-rtl.css`	3.06 kB	0 B
`build/edit-site/style.css`	3.06 kB	0 B
`build/edit-widgets/index.js`	9.38 kB	0 B
`build/edit-widgets/style-rtl.css`	2.45 kB	0 B
`build/edit-widgets/style.css`	2.45 kB	0 B
`build/editor/editor-styles-rtl.css`	537 B	0 B
`build/editor/editor-styles.css`	539 B	0 B
`build/editor/index.js`	45.3 kB	0 B
`build/editor/style-rtl.css`	3.8 kB	0 B
`build/editor/style.css`	3.79 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.72 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	621 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.11 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.33 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.41 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/rich-text/index.js`	13.9 kB	0 B
`build/server-side-render/index.js`	2.71 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4.06 kB	0 B
`build/viewport/index.js`	1.85 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

* Simplify overall logic * Prioritize base block types, excluding those superseded by isDefault:true block variations

mcsf · 2020-08-11T18:31:02Z

@ntsekouras: following our convo on Slack, I pushed a couple of commits to extend test coverage to default block variations and to ensure consistent business logic in includeVariationsInInserterItems. Let me know what you think.

ntsekouras · 2020-08-11T20:09:33Z

Thanks @mcsf! Great simplification 👍 . I just added again the quick exit if items are enough to avoid any iteration at all, given that this is called many times, for example with every search term change. Do you agree?
Of course we could also add there code to replace an item with their default variation, if exists.

I think we are just missing a case for prioritising the base blocks over variations in case there are fewer or equal items with the limit and there are variations ( no default ) in a block before a block with default variation.

I believe it makes it pretty complex to handle this and I don't know if it's worth it TBH.

In general there must be some work on improving/enhancing how block variations work and integrate with the other parts of the editor.

packages/block-editor/src/components/inserter/utils.js

mcsf · 2020-08-12T13:00:36Z

I just added again the quick exit if items are enough to avoid any iteration at all, given that this is called many times, for example with every search term change. Do you agree?
Of course we could also add there code to replace an item with their default variation, if exists.

Hm... my idea is that we shouldn't be trying to optimise without knowing what needs optimisation. That's because in this function it's clear that, the more straightforward and consistent it is, the more confidence we have in the results. Otherwise (as I commented here) when we optimise we give special treatments to certain values and forget to account for all the business rules.

In practice, even if we optimise, we will still need to loop over items to exclude types that have default variations. So that part can't be short-circuited. So the only thing left is the branching at if ( filteredItems.length < limit ) {, which doesn't cost anything. What else would you change?

I think we are just missing a case for prioritising the base blocks over variations in case there are fewer or equal items with the limit and there are variations ( no default ) in a block before a block with default variation.

Is that still the case now that we have return [ ...filteredItems, ...variationsToAdd ].slice( 0, limit );? Maybe I didn't understand your point. If so, can you write a unit test?

ntsekouras · 2020-08-12T13:32:01Z

In practice, even if we optimise, we will still need to loop over items to exclude types that have default variations. So that part can't be short-circuited.

Hm... my idea is that we shouldn't be trying to optimise without knowing what needs optimisation.

I know that @mcsf and I agree and that's why I don't know how much effort we should put in this right now. It should be handled with all the integration of block variations and find out what is needed and what will work best.

Even my suggestion for prioritizing the base blocks would increase the complexity a lot without knowing if this will be the final direction for this.

I tried some work on prioritzing yesterday but needed feedback whether we should implement this or not. Just for reference an example test is below. Failing test because it removes the withDefaultVariationItem and when starts the loop adds the first variation of withSingleVariationItem.

               it( 'should prioritize base block elements', () => {
			const items = [ withSingleVariationItem, withDefaultVariationItem ];
			const res = includeVariationsInInserterItems( items, 2 );
			expect( res ).toEqual( [
				withSingleVariationItem,
				expect.objectContaining( {
					// id: 'core/embed-youtube',
					id: 'core/block-with-default-variation-special', // I believe this would be the expected result?
				} ),
			] );
		} );

Maybe keep your version for now, remove the last addition of mine and rethink/revisit the block variations? For now in master if someone searches for embed will have more results than expected.

What do you think?

mcsf · 2020-08-12T16:23:40Z

Thanks for your exploring and quick reply!

Maybe keep your version for now, remove the last addition of mine and rethink/revisit the block variations? For now in master if someone searches for embed will have more results than expected.

Yeah, I agree with keeping it simple and not investing too much in the extra requirements. Let's fix master.

mcsf

🚢

fill with available variations if limit is set

6ce8f73

ntsekouras added [Type] Bug An existing feature does not function as intended [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Feature] Block Variations Block variations, including introducing new variations and variations as a feature labels Aug 11, 2020

ntsekouras requested review from ellatrix and youknowriad as code owners August 11, 2020 12:18

ntsekouras self-assigned this Aug 11, 2020

ntsekouras requested a review from mcsf August 11, 2020 12:19

mcsf added 2 commits August 11, 2020 19:16

Inserter: Test for isDefault:true block variations

1684c6e

Fix provision of variations in block inserter items

9d4f1cf

* Simplify overall logic * Prioritize base block types, excluding those superseded by isDefault:true block variations

ntsekouras force-pushed the fix/limit-quick-inserter-results branch from 12fc7da to 9d4f1cf Compare August 11, 2020 20:27

add quick exit if there are enough items

e2cf8ec

mcsf reviewed Aug 12, 2020

View reviewed changes

packages/block-editor/src/components/inserter/utils.js Outdated Show resolved Hide resolved

remove incomplete limit check on top

4b820bc

mcsf approved these changes Aug 12, 2020

View reviewed changes

ntsekouras merged commit 21a8791 into master Aug 12, 2020

ntsekouras deleted the fix/limit-quick-inserter-results branch August 12, 2020 16:29

github-actions bot added this to the Gutenberg 8.8 milestone Aug 12, 2020

ItsJonQ mentioned this pull request Aug 17, 2020

Bump plugin version to 8.8.0 #24601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill quick inserter results with available variations if limit is set and items are less. #24481

Fill quick inserter results with available variations if limit is set and items are less. #24481

ntsekouras commented Aug 11, 2020

github-actions bot commented Aug 11, 2020 •

edited

Loading

mcsf commented Aug 11, 2020

ntsekouras commented Aug 11, 2020 •

edited

Loading

mcsf commented Aug 12, 2020

ntsekouras commented Aug 12, 2020 •

edited

Loading

mcsf commented Aug 12, 2020 •

edited

Loading

mcsf left a comment

Fill quick inserter results with available variations if limit is set and items are less. #24481

Fill quick inserter results with available variations if limit is set and items are less. #24481

Conversation

ntsekouras commented Aug 11, 2020

Description

Steps to reproduce

Checklist:

github-actions bot commented Aug 11, 2020 • edited Loading

mcsf commented Aug 11, 2020

ntsekouras commented Aug 11, 2020 • edited Loading

mcsf commented Aug 12, 2020

ntsekouras commented Aug 12, 2020 • edited Loading

mcsf commented Aug 12, 2020 • edited Loading

mcsf left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2020 •

edited

Loading

ntsekouras commented Aug 11, 2020 •

edited

Loading

ntsekouras commented Aug 12, 2020 •

edited

Loading

mcsf commented Aug 12, 2020 •

edited

Loading