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

Persist recently used blocks, and record block usage #2836

Merged
merged 5 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@notnownikki
Member

notnownikki commented Sep 29, 2017

Description

Loading up a new post populated the recently used blocks with
blocks from the common category. This change persists the
recently used blocks, so they are there next time.

This also keeps a "highscore table" of block usage, so in
a future PR, we can use this data to display the user's
most frequently used blocks next to the inserter, where we
currently hardcode paragraph and image. The work to do
that will be in a future PR so that when it's released, we
have a better chance of the user having usage data that
is meaningful to base it on.

How Has This Been Tested?

Add some blocks to a post, they should appear at the
top of the recently used blocks.

Start a new post, or load an existing post, and the blocks in
the recent tab should show the blocks you used most
recently.

Types of changes

New feature (non-breaking change which adds functionality)

Populate recently used blocks based on block usage frequency
Loading up a new post populated the recently used blocks with
blocks from the common category. This changes to keep track
(in local storage) of how much you use each block, and populates
the recently used blocks with the ones you use most frequently
when the editor starts up.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 29, 2017

Codecov Report

Merging #2836 into master will decrease coverage by 0.19%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2836     +/-   ##
=========================================
- Coverage   33.77%   33.58%   -0.2%     
=========================================
  Files         191      191             
  Lines        5691     5810    +119     
  Branches      996     1023     +27     
=========================================
+ Hits         1922     1951     +29     
- Misses       3189     3259     +70     
- Partials      580      600     +20
Impacted Files Coverage Δ
editor/store-defaults.js 100% <ø> (ø) ⬆️
editor/selectors.js 96.75% <0%> (ø) ⬆️
editor/reducer.js 88.96% <100%> (+0.39%) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/library/paragraph/index.js 34.78% <0%> (+1.44%) ⬆️
blocks/library/cover-image/index.js 35.89% <0%> (+5.46%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db507cd...6fad6be. Read the comment docs.

codecov bot commented Sep 29, 2017

Codecov Report

Merging #2836 into master will decrease coverage by 0.19%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2836     +/-   ##
=========================================
- Coverage   33.77%   33.58%   -0.2%     
=========================================
  Files         191      191             
  Lines        5691     5810    +119     
  Branches      996     1023     +27     
=========================================
+ Hits         1922     1951     +29     
- Misses       3189     3259     +70     
- Partials      580      600     +20
Impacted Files Coverage Δ
editor/store-defaults.js 100% <ø> (ø) ⬆️
editor/selectors.js 96.75% <0%> (ø) ⬆️
editor/reducer.js 88.96% <100%> (+0.39%) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/library/paragraph/index.js 34.78% <0%> (+1.44%) ⬆️
blocks/library/cover-image/index.js 35.89% <0%> (+5.46%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db507cd...6fad6be. Read the comment docs.

let recentlyUsedBlocks = [ ...state.recentlyUsedBlocks ];
action.blocks.forEach( ( block ) => {
const uses = ( blockUsage[ block.name ] || 0 ) + 1;
blockUsage = omit( blockUsage, block.name );

This comment has been minimized.

@youknowriad

youknowriad Sep 29, 2017

Contributor

Is this line necessary since we're overriding the value just after?

@youknowriad

youknowriad Sep 29, 2017

Contributor

Is this line necessary since we're overriding the value just after?

This comment has been minimized.

@notnownikki

notnownikki Sep 29, 2017

Member

I thought this is part of the principle of always creating a new values, rather than changing existing data. The state should be deepFreeze'd here in tests, so trying to change blockUsage should result in an error, because we'd be modifying the state directly.

@notnownikki

notnownikki Sep 29, 2017

Member

I thought this is part of the principle of always creating a new values, rather than changing existing data. The state should be deepFreeze'd here in tests, so trying to change blockUsage should result in an error, because we'd be modifying the state directly.

Show outdated Hide outdated editor/reducer.js Outdated
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2017

Contributor

Really nice work, thanks for working on this.

Took the branch for a spin, did this:

  • Looked at inserter, saw paragraph first
  • Inserted Latest Posts, saw latest posts sorted at the top

This felt expected, very nice! Then created a new post, saw the Latest Posts block sorted not at first, like it was previously. That was a bit confusing.

Perhaps we do just want to persist the recently used list itself? If someone uses a block frequently, it's unlikely to drop out of the list, although it may change position, as we put the most recently used ones at the top. Or perhaps it's always a list of the most frequently used blocks, ordered by how often they're used?

This sounds right to me. That is — if I keep using the "Recent Posts" block, it will ride up the "highscore list" all the way to the top.

The key here is that the recent lists changes per your usage, but persists so it's "the same" in between posts, and only changes when you actually use other blocks.

Does that make sense? Did I grok this right?

Contributor

jasmussen commented Oct 2, 2017

Really nice work, thanks for working on this.

Took the branch for a spin, did this:

  • Looked at inserter, saw paragraph first
  • Inserted Latest Posts, saw latest posts sorted at the top

This felt expected, very nice! Then created a new post, saw the Latest Posts block sorted not at first, like it was previously. That was a bit confusing.

Perhaps we do just want to persist the recently used list itself? If someone uses a block frequently, it's unlikely to drop out of the list, although it may change position, as we put the most recently used ones at the top. Or perhaps it's always a list of the most frequently used blocks, ordered by how often they're used?

This sounds right to me. That is — if I keep using the "Recent Posts" block, it will ride up the "highscore list" all the way to the top.

The key here is that the recent lists changes per your usage, but persists so it's "the same" in between posts, and only changes when you actually use other blocks.

Does that make sense? Did I grok this right?

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 2, 2017

Member

@jasmussen it's a little bit more subtle...

This sounds right to me. That is — if I keep using the "Recent Posts" block, it will ride up the "highscore list" all the way to the top.

We can't use the "highscore" by itself. The problem is, if we have 8 blocks we use a lot, then we use one we've never used before, it won't have a score high enough to appear in the list.

If we use the "last used time" by itself, then the list becomes purely a list of what we've used recently, with most recent at the top, which I think is what people are expecting, although the list would change order depending on what blocks you've used most recently. However, this is exactly how a "recently opened documents" list works, so perhaps what people expect.

From the feedback I'm getting here, it seems like the "highscore" isn't of any use, and we should just be persisting the list based on when blocks were last used.

Member

notnownikki commented Oct 2, 2017

@jasmussen it's a little bit more subtle...

This sounds right to me. That is — if I keep using the "Recent Posts" block, it will ride up the "highscore list" all the way to the top.

We can't use the "highscore" by itself. The problem is, if we have 8 blocks we use a lot, then we use one we've never used before, it won't have a score high enough to appear in the list.

If we use the "last used time" by itself, then the list becomes purely a list of what we've used recently, with most recent at the top, which I think is what people are expecting, although the list would change order depending on what blocks you've used most recently. However, this is exactly how a "recently opened documents" list works, so perhaps what people expect.

From the feedback I'm getting here, it seems like the "highscore" isn't of any use, and we should just be persisting the list based on when blocks were last used.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2017

Contributor

It's a bit above my head, so I would defer to Riad here — but just wanted to note your work here is really appreciated, thank you.

Contributor

jasmussen commented Oct 2, 2017

It's a bit above my head, so I would defer to Riad here — but just wanted to note your work here is really appreciated, thank you.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 2, 2017

Member

I really think that we just persist the recently used state. Mixing the highscore table and the recently used state is just confusing.

HOWEVER, I think it's still worth keeping the highscore table (adopting that term, I like it haha) because the quick links next to the inserter icon (currently for paragraph and image) could be generated by the blocks a user most frequently uses, but that can come in a future PR.

Member

notnownikki commented Oct 2, 2017

I really think that we just persist the recently used state. Mixing the highscore table and the recently used state is just confusing.

HOWEVER, I think it's still worth keeping the highscore table (adopting that term, I like it haha) because the quick links next to the inserter icon (currently for paragraph and image) could be generated by the blocks a user most frequently uses, but that can come in a future PR.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 2, 2017

Member

I've changed this around now so it only uses the blocks you have used recently for the recently used blocks tab, displaying them with most recent at the top.

The block usage highscore is still in there, once we're happy with this branch and it's merged, I'll do a separate PR for the frequently used blocks to appear next to the inserter, where paragraph and image currently are.

Member

notnownikki commented Oct 2, 2017

I've changed this around now so it only uses the blocks you have used recently for the recently used blocks tab, displaying them with most recent at the top.

The block usage highscore is still in there, once we're happy with this branch and it's merged, I'll do a separate PR for the frequently used blocks to appear next to the inserter, where paragraph and image currently are.

Show outdated Hide outdated editor/reducer.js Outdated

@notnownikki notnownikki changed the title from Populate recently used blocks based on block usage frequency to Persist recently used blocks, and record block usage Oct 3, 2017

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 3, 2017

Member

Alrighty, I have updated the title and description here to reflect the outcome of our discussions. Thanks for all the reviews so far! Hopefully this makes sense now, and we can get it out there and start populating the highscore tables :)

Member

notnownikki commented Oct 3, 2017

Alrighty, I have updated the title and description here to reflect the outcome of our discussions. Thanks for all the reviews so far! Hopefully this makes sense now, and we can get it out there and start populating the highscore tables :)

@youknowriad

Good job 👍

Show outdated Hide outdated editor/reducer.js Outdated
Show outdated Hide outdated editor/reducer.js Outdated
Show outdated Hide outdated editor/reducer.js Outdated

@notnownikki notnownikki merged commit 8908351 into master Oct 3, 2017

3 checks passed

codecov/project Absolute coverage decreased by -0.19% but relative coverage increased by +60.34% compared to db507cd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mtias mtias deleted the update/recent-blocks-based-on-usage branch Oct 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment