Recent blocks #1694

notnownikki · 2017-07-04T10:29:33Z

Refactored code that generates and sorts visible blocks so that
the order of recent blocks can be arbitrary, and recent blocks
appear in their own category.

Maximum of 8 recent blocks are shown, with most recently used
at the top. This is not persisted between editor sessions, there
is a TODO item in the usage module to highlight this needs to
be done.

The buttons in block-list which currently show core/text and core/image
have not been modified to use the recently used blocks yet,
this is to be looked at in the next iteration.

jasmussen · 2017-07-05T10:47:58Z

Love it!

Notably after using the Classic Text once, it starts showing up in the Recent Blocks list, which is cool.

Perhaps not for this PR, but I know that @folletto had some ideas for how to best do "recent blocks" so that we don't mess too much with spatial memory. Right now the most recently used block is moved to the top, which can cause you to have to look for where the Image block is, because it switches places very often.

A very basic way to do this, is to simply say blocks are sorted alphabetically on the Recent tab, and only show 8 blocks total. So if you have 8 blocks already and use a new block, the least used of the old ones pops off the list.

notnownikki · 2017-07-05T10:51:49Z

Yeah, the algorithm for "recently used" is very, VERY basic here. I wanted this reviewed because the main body of work was getting the inserter to work with arbitrary lists of blocks, and I want to move the logic behind what appears in the recent tab into an API call so that we can initially fill it with the user's most frequently used blocks when they start a document, and then do the recently used behaviour.

youknowriad · 2017-07-05T18:04:35Z

blocks/api/usage.js

+export function recordUsage( blockType ) {
+	recentBlockNames = [
+		blockType.name,
+		...recentBlockNames.filter( name => name !== blockType.name ),


Could we use lodash's without utility?

is there nothing lodash doesn't do? 😄

youknowriad · 2017-07-05T18:04:40Z

blocks/api/usage.js

+		blockType.name,
+		...recentBlockNames.filter( name => name !== blockType.name ),
+	].slice( 0, maxRecent );
+}


Should we unit test these methods?

I was going to, but they will be gone in the next PR, which stores and calculates these on the server side, and is unit tested

youknowriad · 2017-07-05T18:05:31Z

blocks/api/usage.js

+ */
+import { getBlockType, getBlockTypes } from './registration';
+
+let recentBlockNames = [];


Should we store these in the redux state instead?

Sounds like we should, this isn't internal block state or anything... Are we ok with doing this in the next iteration? The majority of the code in the usage module is going to be replaced with calls to a server side API so we can store a user's block usage (and do things like, initially populate the recently used blocks with their most frequently used blocks).

mtias · 2017-07-08T00:32:13Z

editor/inserter/menu.js

+		return filter( blockTypes, matchesSearch );
+	}
+
+	getBlockTypesForCurrentTab() {


I suggest getBlocksForCurrentTab here

mtias · 2017-07-08T00:32:42Z

editor/inserter/menu.js

-	sortBlocksByCategory( blockTypes ) {
+	sortBlocks( blockTypes ) {
+		if ( 'recent' === this.state.tab ) {
+			return blockTypes; // already sorted by getRecent() according to recent-ness


comment should be in the line above

mtias · 2017-07-08T00:34:10Z

editor/inserter/menu.js

 		const getCategoryIndex = ( item ) => {
 			return findIndex( getCategories(), ( category ) => category.slug === item.category );
 		};

 		return sortBy( blockTypes, getCategoryIndex );
 	}

+	addRecent( blocksByCategory ) {


this function name could be more clear, it can be read as "add block to recent"

mtias · 2017-07-08T00:35:28Z

editor/inserter/menu.js

@@ -269,22 +297,19 @@ class InserterMenu extends Component {
 					tabIndex="-1"
 				/>
 				<div role="menu" className="editor-inserter__content">
-					{ this.state.tab === 'recent' &&
+					{ this.state.tab === 'recent' && (


the parenthesis shouldn't be needed

mtias · 2017-07-08T20:55:07Z

blocks/api/usage.js

+const maxRecent = 8;
+
+// TODO: replace storage of recent blocks in this module with calls to an API
+export function getRecent() {


It would be good to include "block" in the function name: getRecentBlocks.

mtias · 2017-07-08T20:55:50Z

editor/inserter/menu.js

 		return () => {
-			this.props.onSelect( name );
+			this.props.onSelect( blockType.name );
+			recordUsage( blockType );


What about using the INSERT_BLOCK action instead, which should already be firing?

Yes, that's a much better way.

As that is the beginning of moving things into the actions and app state instead of a module level variable, I'll mark this PR as WIP until that's done and using the REST API to keep a record of block usage.

Sounds good. Though it'd be fine to do the REST API pieces separately, as it's not immediately obvious where this should be stored.

Sounds sensible, I have a branch started locally to explore storage options, so I'll carry on work here and make sure it's easy to switch over to use API calls.

notnownikki · 2017-07-11T09:59:08Z

Major update! Now using the new test framework! Now using the new action type!

Also, recent block usage moved into the application state. There are two actions this hooks into - SETUP_NEW_POST populates the recently used blocks with initial data - currently just the common category, but this will be changed when we have a server side API to populate it with the users most frequently related blocks (or whatever we decide), and of course, recording block usage on INSERT_BLOCKS.

notnownikki · 2017-07-11T14:04:48Z

Back to "in progress" because the search change from master breaks the new tabbing behaviour.

Hopefully I'll get this fixed before we change too much again ;)

notnownikki · 2017-07-11T14:11:25Z

Issue fixed. The problem was that the recent tab takes the order from whatever the application says the ordering is (which is great for when we have some API somewhere doing weighting), but if you searched in the recent tab, the search is now global so the navigation's ordering was wrong.

Simple check in sortBlocks fixed it.

mtias · 2017-07-11T14:31:52Z

One odd behaviour with sorting by title is that as soon as you insert a block all the ordering shuffles for common blocks, which doesn't seem great. Can we just disable the title ordering since we are likely to weight the results in any case?

Refactored code that generates and sorts visible blocks so that the order of recent blocks can be arbitrary, and recent blocks appear in their own category.

mtias · 2017-07-11T17:17:11Z

Closes #888.

mtias · 2017-07-11T17:20:55Z

editor/inserter/menu.js

@@ -58,37 +57,69 @@ class InserterMenu extends Component {
 		} );
 	}

-	selectBlock( name ) {
+	selectBlock( blockType ) {


Why pass blockType if only using the name?

Because we used to store the whole block before that data was moved into the app state. Will change!

mtias · 2017-07-11T17:24:40Z

editor/inserter/menu.js

 		const getCategoryIndex = ( item ) => {
 			return findIndex( getCategories(), ( category ) => category.slug === item.category );
 		};

 		return sortBy( blockTypes, getCategoryIndex );
 	}

+	addRecentBlocks( blocksByCategory ) {


"Add" feels a bit weird in this context. Why does it need to "add" anything? Should recent be a "category" or just returned on demand for the recent tab?

Well, it's "adding" .recent to the object that has the blocks groups by category. We'd need to rework the render method a bit to get rid of this, which I'm happy to do, it's just this PR was getting big enough as it was :)

Just to clarify why this needs a separate step, 'recent' isn't a category because we don't have the ability for a block to be in two categories, so the method that sorts blocks by category has no knowledge of 'recent', and this method is adding the recent category to the object that holds the blocks grouped by category.

To be honest, the render method could use cleaning up, there's a lot of duplication and we could simplify it a lot. Your call if you'd like this done in this PR or not, I'm happy to do it either here, or as another PR.

I am ok refactoring later.

mtias · 2017-07-11T17:25:59Z

editor/inserter/menu.js

+		return blocksByCategory;
+	}
+
+	filterRecentForSearch( blocksByCategory ) {


Not sure I understand why we need this function. Wouldn't search just search all available blocks and list the results?

Ah, I guess this is made redundant by the recent "search searches everything" change. Will remove.

mtias · 2017-07-11T17:26:32Z

editor/inserter/menu.js

 			this.setState( {
 				filterValue: '',
 				currentFocus: null,
 			} );
 		};
 	}

-	getVisibleBlocks( blockTypes ) {
-		return filter( blockTypes, this.isShownBlock );
+	applySearchFilter( blockTypes ) {


This could be just be searchBlocks.

mtias · 2017-07-11T17:27:13Z

editor/inserter/menu.js

@@ -245,12 +276,13 @@ class InserterMenu extends Component {

 	switchTab( tab ) {
 		this.setState( { tab: tab } );
+		this.setSearchFocus();


Why do we need this? Sounds like it could mess with accessibility.

this was to try and make it easier to use for people who are mostly keyboard based. There's no way to switch between tabs with the keyboard right now, so to make it easier to find the block you want, I put the focus into search so you could either start typing the name of a block or start tabbing through the list without any further mouse clicks. Happy to take it out, but it saved a lot of mouse movement when testing search and tabbing!

There again, that was before search was at the top. I'll leave the call to someone who knows better!

Let's remove this for now to not mess with focus. We'll need another pass of keyboard flows here once things settle.

mtias · 2017-07-11T17:29:08Z

editor/state.js

@@ -7,6 +7,11 @@ import refx from 'refx';
 import { reduce, keyBy, first, last, omit, without, flowRight } from 'lodash';

 /**
+ * WordPress dependencies
+ */
+import { getBlockTypes } from 'blocks';


This should be internal dependency.

Could you clarify this? Imports from 'blocks' are WordPress dependencies in other places.

I've just checked, and everywhere else that imports things from 'blocks', it's listed as a WordPress dependency. Does everywhere else need to change, or is this specific to the getBlockTypes function? If so, can you clarify why?

Yes, sorry, my bad, this is assuming how we'll expose these. We would need to fix this one instead: https://github.com/WordPress/gutenberg/pull/1694/files#diff-d9e543404329206d6652e45f824ae35fR11

mtias · 2017-07-11T21:31:26Z

@notnownikki 🚢 this. Nice work!

mtias · 2017-07-11T21:32:10Z

Let's also create an issue to track persistence and weighting the recency.

youknowriad reviewed Jul 5, 2017

View reviewed changes

mtias added [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Jul 8, 2017

mtias reviewed Jul 8, 2017

View reviewed changes

notnownikki added the [Status] In Progress Tracking issues with work in progress label Jul 10, 2017

notnownikki changed the title ~~Simple "recent blocks" implementation (part 1)~~ [WIP] Recent blocks Jul 10, 2017

notnownikki changed the title ~~[WIP] Recent blocks~~ Recent blocks Jul 11, 2017

notnownikki removed the [Status] In Progress Tracking issues with work in progress label Jul 11, 2017

notnownikki added the [Status] In Progress Tracking issues with work in progress label Jul 11, 2017

notnownikki removed the [Status] In Progress Tracking issues with work in progress label Jul 11, 2017

Simple "recent blocks" implementation

8f00652

Refactored code that generates and sorts visible blocks so that the order of recent blocks can be arbitrary, and recent blocks appear in their own category.

notnownikki force-pushed the update/recently-used-blocks branch from e43d896 to f6a2202 Compare July 11, 2017 15:03

Fixed up tests, removed ordering recent blocks by title

ead5309

notnownikki force-pushed the update/recently-used-blocks branch from f6a2202 to ead5309 Compare July 11, 2017 15:05

mtias reviewed Jul 11, 2017

View reviewed changes

notnownikki added 3 commits July 11, 2017 18:40

Removed redundant search method, clarified some other method names

e455346

Remove focus search on switching tab

9286b68

Fix dependency declaration comment

1ef4308

notnownikki merged commit 278b499 into master Jul 11, 2017

notnownikki deleted the update/recently-used-blocks branch July 11, 2017 21:33

noisysocks mentioned this pull request Mar 22, 2018

Consider not padding Frequent tab with blocks that are in common category #5627

Closed

Recent blocks #1694

Recent blocks #1694

Conversation

notnownikki commented Jul 4, 2017

jasmussen commented Jul 5, 2017

notnownikki commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Jul 11, 2017

notnownikki commented Jul 11, 2017

notnownikki commented Jul 11, 2017

mtias commented Jul 11, 2017

mtias commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented Jul 11, 2017

mtias commented Jul 11, 2017