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

Exclude reusable blocks from the global block count #11787

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

noisysocks
Copy link
Member

Fixes #11744.

Modifies getGlobalBlockCount() to exclude reusable blocks from its count. This fixes the block count including reusable blocks that have been parsed and stored into blocks.byClientId but not inserted into the post or page.

How to test

  1. Create a new post
  2. Insert a block
  3. Convert it to a reusable block
  4. Click Content Structure
  5. It should report that there is 1 block

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Nov 13, 2018
@noisysocks noisysocks added this to the 4.4 milestone Nov 13, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @noisysocks, this seems an improvement and addresses some problems.
But we need to take into consideration the descendants of the child blocks.
If we convert a columns block with two columns and with one paragraph inside each column to a reusable block, the columns block is not counted but the 2 "column" blocks and 2 paragraph blocks are counted so we have an additional four blocks in the global block count.
We are also taking into account all paragraphs and headings inside reusable blocks in the global headings and paragraphs counts.

@mtias
Copy link
Member

mtias commented Nov 13, 2018

We are also taking into account all paragraphs and headings inside reusable blocks in the global headings and paragraphs counts.

I'd consider this a nice to have and not high priority. The most important is to fix the surplus in the counts.

@noisysocks noisysocks force-pushed the fix/global-block-count-includes-reusable-blocks branch from 488bb0d to 30bcbdb Compare November 14, 2018 00:46
@noisysocks
Copy link
Member Author

Thanks Jorge! Good catch. I've modified the selector to use getClientIdsWithDescendants which does exactly what we need here.

@noisysocks noisysocks force-pushed the fix/global-block-count-includes-reusable-blocks branch from 30bcbdb to a248a9d Compare November 14, 2018 01:13
Modifies getGlobalBlockCount() to exclude reusable blocks from its
count. This fixes the block count including reusable blocks that have
been fetched and parsed but not inserted into the post or page.
@noisysocks noisysocks force-pushed the fix/global-block-count-includes-reusable-blocks branch from a248a9d to 7e70858 Compare November 14, 2018 01:28
@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018
},
( state ) => [
state.editor.present.blocks.order,
state.editor.present.blocks.byClientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really worth memoizing selectors if they depend on these two elements particularly? I feel state.editor.present.blocks.byClientId changes so often that it's just a waste of time to memoize these selectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on this @aduth?

Copy link
Member Author

Choose a reason for hiding this comment

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

But it seems wasteful to have this selector run when something unrelated e.g. a post attribute is modified, no?

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 know honestly, it feels like typing in blocks is the most impactful thing in terms of performance, changing an attribute only get triggered once and not extensively.

Anyway, just something to keep in mind for now, there's no clear "winner"

@gziolo gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018
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.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content Structure: Reusable blocks that aren't in the page are counted
5 participants