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

Refactor InserterMenu & VisualEditorInserter; allow recent/frequent reusable blocks to be inserted #4224

Closed
wants to merge 4 commits into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jan 2, 2018

🕺 What

Currently, InserterMenu, VisualEditorInserter, and blocksAutocompleter all directly call getBlockTypes() and getReusableBlocks() to determine what blocks should be shown as something that can be inserted.

This means that we duplicate some logic in three different places:

  1. Checking, if the block has useOnce set, whether it's been already used or not
  2. Checking whether or not the block has isPrivate set
  3. Checking whether or not the block is in the editor config's list of enabled block types
  4. Mixing in the list of available Reusable Blocks with the list of available static blocks

This approach is pretty error-prone: it's very easy to change the behaviour of one inserter and forget about the other three inserters.

To start addressing this, I've changed InserterMenu and VisualEditorInserter to source their blocks from three new selectors:

  • getInserterItems - returns all items that are insertable, filters out private and disabled blocks, correctly sets isDisabled
  • getRecentInserterItems - returns recently inserted items (e.g. for use in the Recent tab), filters out private and disabled blocks, correctly sets isDisabled
  • getFrequentInserterItems - returns frequently inserted items (e.g. for use in VisualEditorInserter), filters out private and disabled blocks, correctly sets isDisabled

All three selectors select from both dynamic (reusable) blocks and static blocks. That means that we fix #3793 and fix #4221.

Having one place to assemble how a reusable block appears in an inserter lets us also easily fix #4303.

📅 Future work

The next step is to make blocksAutocompleter pull from getInserterItems. I've not done this yet as doing so would rely on being able to access editor state from the blocks module—something that we haven't figured out yet.

✅ How to test

  1. Insert some static blocks and reusable blocks
  2. Recently inserted blocks should move to the top of the Recent tab in the main inserter
  3. Frequently inserted blocks should appear at the bottom of the visual editor
    1. Double check that a block can be inserted by clicking one of these items
  4. Recent and frequent blocks should persist if you create a new post

@noisysocks noisysocks added [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Status] In Progress Tracking issues with work in progress labels Jan 2, 2018
@aduth
Copy link
Member

aduth commented Jan 3, 2018

Thoughts on this approach, @youknowriad and @aduth?

If insertable blocks are largely state-derived, I agree it's sensible that they could be implemented as selectors.

One potential issue I see with the current implementation is with memoization via createSelector. Since we're only busting the cache on changes to state, but not on newly-registered block types, we might run into issues where blocks don't appear correctly. This could be accounted for by returning getBlockTypes() as part of the second argument of createSelector.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 4, 2018

Note from #4139 (review): We should remove the reusable block from recentInserts and insertsFrequency when a reusable block is deleted.

@noisysocks noisysocks force-pushed the try/inserter-refactor branch 2 times, most recently from 82d95a1 to 641f983 Compare January 5, 2018 05:41
@noisysocks noisysocks changed the title [WIP] Refactor the inserters; allow reusable blocks to be searched Refactor InserterMenu & VisualEditorInserter; allow recent/frequent reusable blocks to be inserted Jan 5, 2018
@noisysocks noisysocks removed the [Status] In Progress Tracking issues with work in progress label Jan 5, 2018
@noisysocks
Copy link
Member Author

noisysocks commented Jan 5, 2018

✨ This is ready to be properly reviewed now.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 5, 2018

This could be accounted for by returning getBlockTypes() as part of the second argument of createSelector.

I looked into this and createSelector relies on one passing immutable objects as the second argument. This means that passing in getBlockTypes() would cause the selector to always be invalidated.

My gut says that it's OK to leave this as is since registering a block type is unlikely to happen after the editor is instantiated—no?

@noisysocks noisysocks force-pushed the try/inserter-refactor branch from 641f983 to fb2669f Compare January 7, 2018 23:45
@noisysocks noisysocks force-pushed the try/inserter-refactor branch from ef4dc56 to e73eb4a Compare January 11, 2018 04:25
Changes the main inserter and the quick inserter to both use common
selectors which determine which items that should appear for insertion.
This means that common logic (e.g. is this block enabled?, has this
block already been used?) can be shared between the two inserters.

At the same time, also allow Reusable Blocks to appear in the MRU and
MFU inserter lists.
When a reusable block is deleted, remove that block from recentInserts
and frequentInserts.
Use the underlying block type's icon as the icon that displays in an
inserter for a reusable block.
@noisysocks noisysocks force-pushed the try/inserter-refactor branch from e73eb4a to d884f71 Compare January 12, 2018 01:50
@noisysocks noisysocks requested a review from gziolo January 12, 2018 04:26
@noisysocks
Copy link
Member Author

Going to split this work into smaller PRs that are less intimidating to review.

@noisysocks noisysocks closed this Jan 16, 2018
@aduth aduth deleted the try/inserter-refactor branch January 16, 2018 15:33
@aduth
Copy link
Member

aduth commented Jan 16, 2018

Going to split this work into smaller PRs that are less intimidating to review.

See #4497

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)
Projects
None yet
2 participants