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

Replace the side inserter by an inserter with shortcuts on empty paragraphs #4953

Merged
merged 8 commits into from
Feb 13, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Feb 8, 2018

closes #4951

This PR updates the side inserter. Instead of having a side inserter at the right of each block we show a quick inserter with shortcuts for the most frequently used blocks on empty paragraphs.

Combined with the default block appender, this allows a nice writing flow, where you click the empty area at the bottom of a post to create a new empty paragraph which will trigger this side inserter.

@youknowriad youknowriad added the [Status] In Progress Tracking issues with work in progress label Feb 8, 2018
@youknowriad youknowriad self-assigned this Feb 8, 2018
@jasmussen
Copy link
Contributor

Amazingly fast work. Also closes #4954. Taking a look.

@jasmussen
Copy link
Contributor

This is already feeling much better than anything we've done in the past.

@paulwilde
Copy link
Contributor

paulwilde commented Feb 8, 2018

I think the hover styles should be rounded like they are in menu at the top. Seems to be 4px.

screen shot 2018-02-08 at 19 23 47

screen shot 2018-02-08 at 19 27 54

@@ -627,12 +634,17 @@ export function preferences( state = PREFERENCES_DEFAULTS, action ) {
insert,
...reject( prevState.recentInserts, isSameAsInsert ),
],
insertUsage: {
...prevState.insertUsage,
[ id ]: usage,
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be slightly clearer if we remove lines 624-627 and increment the count right here:

insertUsage: {
    ...prevState.insertUsage,
    [ id ]: {
        count: prevState.insertUsage[ id ] ? prevState.insertUsage[ id ].count + 1 : 1,
        insert,
    },
},

}

/**
* Determines the items that appear in the inserter with shortcodes based on the block usage
Copy link
Member

Choose a reason for hiding this comment

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

Typo: s/shortcodes/shortcuts/

* @return {Editor.InserterItem[]} Items that appear in the 'Recent' tab.
*/
export function getRecentInserterItems( state, enabledBlockTypes = true ) {
function getItemsFromInserts( state, inserts, enabledBlockTypes = true, maximum = MAX_RECENT_BLOCKS ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should we get rid of MAX_RECENT_BLOCKS and force callers to just always pass in a maximum.

*
* @return {Editor.InserterItem[]} Items that appear in the 'Recent' tab.
*/
export function getMostFrequentlyUsedBlocks( state, enabledBlockTypes = true, maximum = MAX_RECENT_BLOCKS ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would getFrequentInserterItems be a better name for this? Then there would be a nice duality between this function and getRecentInserterItems.

},
};

// We should get back 8 items with no duplicates
Copy link
Member

Choose a reason for hiding this comment

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

Typo: s/8/4/

return null;
}

const itemsWithTheDefaultBlock =
Copy link
Member

Choose a reason for hiding this comment

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

Should this be named itemsWithoutDefaultBlock?

export default compose(
connect(
( state ) => ( {
items: getMostFrequentlyUsedBlocks( state, true, 3 ),
Copy link
Member

Choose a reason for hiding this comment

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

We should pass in the proper enabledBlockTypes setting instead of true here.

}

const itemsWithTheDefaultBlock =
filter( items, ( item ) => item.name !== getDefaultBlockName() || ! item.initialAttributes )
Copy link
Member

Choose a reason for hiding this comment

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

! item.initialAttributes will always evaluate to false. I think this should be:

const itemsWithoutDefaultBlock = filter( items, ( item ) =>
    item.name !== getDefaultBlockName() || Object.keys( item.initialAttributes ).length > 0
).slice( 0, 2 );

Using without and isEmpty would help make it read more naturally:

const itemsWithoutDefaultBlock = without( items, ( item ) =>
    item.name === getDefaultBlockName() && isEmpty( item.initialAttributes )
).slice( 0, 2 );

@noisysocks
Copy link
Member

Just realised this has the In Progress label. Apologies for butting in so early with code comments 🙂 —it's looking really good!

@youknowriad
Copy link
Contributor Author

@noisysocks No problem, it's always good to have feedback. And I would have missed most of these ;). Thanks

@mtias
Copy link
Member

mtias commented Feb 9, 2018

@youknowriad one more thing to look at is the blue inserter line when you click on the + button — it should not be there as you'd be replacing the existing block instead:

image

Also, related to 4492, none of the block UI should be showing when clicking on the + button.

@mellis32
Copy link

mellis32 commented Feb 9, 2018

Will the inserter shortcuts be grayed out until hovered as in #4951 and Dropbox Paper? I like the subtly of that apperance, which puts the focus back on the content.

@youknowriad
Copy link
Contributor Author

I made some tweaks here, this should be closer to merge now. I'd love to have some input.

@youknowriad youknowriad dismissed noisysocks’s stale review February 12, 2018 16:14

Feedback addressed

@youknowriad youknowriad removed the [Status] In Progress Tracking issues with work in progress label Feb 13, 2018
@mtias
Copy link
Member

mtias commented Feb 13, 2018

@youknowriad this is working really well for me. If there's one last thing to add, it'd be to try not showing the block outline, the paragraph toolbar, and the ellipsis menu when the paragraph block is empty.

@youknowriad
Copy link
Contributor Author

youknowriad commented Feb 13, 2018

Got a DM approval for this one cc @mtias . Merging to be ready for the release.

There's still an edge-case where the onChange is not triggered properly (clicking backspace while there's a selection, I guess TinyMCE should trigger input in this case. I'm considering this minor and should not be blocking this PR)

@youknowriad youknowriad merged commit 15d5ff2 into master Feb 13, 2018
@youknowriad youknowriad deleted the update/side-inserter branch February 13, 2018 13:19
@jasmussen
Copy link
Contributor

🎉🔥

@aduth
Copy link
Member

aduth commented Feb 14, 2018

I really like this iteration.

One issue I've noticed: It's not possible to expand the "More Options" block ellipsis menu with an empty default block.

Potentially related to #4872 (comment)

If it is, maybe we should evaluate whether that auto-focus logic is worthwhile?

@youknowriad
Copy link
Contributor Author

@aduth I don't it's related to #4872, it's more related to this comment #4953 (comment) and this commit dbad796

It's a bit tricky though, because we want the empty paragraph block to always appear unselected while allowing these menus on hover.

}

const itemsWithoutDefaultBlock = filter( items, ( item ) =>
item.name !== getDefaultBlockName() || ! isEmpty( item.initialAttributes )
Copy link
Member

Choose a reason for hiding this comment

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

Why ! isEmpty( item.initialAttributes ) ?

Copy link
Member

Choose a reason for hiding this comment

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

It's so that shared paragraph blocks don't get filtered out.

Copy link
Member

Choose a reason for hiding this comment

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

As a casual observer, I'd have no hints from the context available to reach this conclusion. Even now, I'm not clear what it is about shared blocks that causes us to filter them.

I'd suggest some combination of code comment and/or explaining variable.

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.

Revisit the trailing inserter
7 participants