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

Try: use 'frecency' to sort items in the inserters #5342

Merged
merged 2 commits into from
Mar 5, 2018
Merged

Conversation

noisysocks
Copy link
Member

Closes #5320.

screen shot 2018-03-01 at 10 58 10

Quick attempt at using frecency—a heuristic that combines frequency and recency—to sort the items that appear in both the main inserter and the quick inserter.

The scoring function I used is borrowed from z, an open source command line tool. We'll likely want to adjust the algorithm based on what feels right.

To test, insert a few different blocks and look at how that affects the Recent tab in the inserter. Recently inserted blocks should move to the top of the list, but they shouldn't displace very frequently used blocks.

Use a heuristic that combines frequency and recency to sort the items
that appear in the two inserters. The scoring function used is borrowed
from z, an open source command line tool: https://github.com/rupa/z
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.

Code wise, this is looking good. I'll defer to @jasmussen and @karmatosed for the algorithm.

@jasmussen
Copy link
Contributor

This seems good to me, though it's hard to test quickly as almost by definition we have to let this run for a while to test it. 👍 👍

@mtias
Copy link
Member

mtias commented Mar 2, 2018

Should we change the label?

@karmatosed
Copy link
Member

karmatosed commented Mar 2, 2018

Should we change the label?

Yes good point, it's not recent anymore. Stumbling as to what though. Maybe we just don't have a label and say blocks, then library?

@folletto
Copy link
Contributor

folletto commented Mar 2, 2018

It's too long, but for reference Slack uses "Frequently Used".

@youknowriad
Copy link
Contributor

What about just "Frequent" to make it shorter?

@jasmussen
Copy link
Contributor

Frequent 👍

(Or "Frecent"? 😉)

@StaggerLeee
Copy link

"Popular"

@noisysocks
Copy link
Member Author

I've renamed the tab to Frequent 🙂

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.

Nice 👍

@noisysocks noisysocks merged commit d459ee9 into master Mar 5, 2018
@noisysocks noisysocks deleted the try/frecency branch March 5, 2018 22:22
insertUsage: {
...prevState.insertUsage,
[ id ]: {
time: Date.now(),
Copy link
Member

@aduth aduth Mar 5, 2018

Choose a reason for hiding this comment

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

Pedantry: Date.now makes this reducer impure:

https://en.wikipedia.org/wiki/Functional_programming#Pure_functions

If a pure function is called with arguments that cause no side-effects, the result is constant with respect to that argument list ... i.e., if calling the pure function again with the same arguments returns the same result.

Violating a core principle of Redux:

Reducers are just pure functions that take the previous state and an action, and return the next state.

https://redux.js.org/introduction/three-principles#changes-are-made-with-pure-functions

Copy link
Member Author

Choose a reason for hiding this comment

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

You're totally right. I'll move this to the action creator.

insertUsage: {
'core-embed/twitter': {
time: expect.any( Number ),
Copy link
Member

Choose a reason for hiding this comment

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

To the above point, we shouldn't have any need for expect.any in this file, given that a reducer should be deterministic.

@aduth
Copy link
Member

aduth commented Mar 8, 2018

Out of curiosity, since preferences is persisted to localStorage, can we assume this will gracefully upgrade for users who already had the value stored in previous versions of the plugin?

Wondering about some of the selector arithmetic around Date.now() - time where time could be undefined.

@noisysocks
Copy link
Member Author

Good point. duration will be NaN which means that calculateFrecency will fall through to the default case and treat the block as if it were used a long long time ago. This isn't incorrect per se, but I'll update the function to more explicitly handle this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants