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

Improve child blocks API's and UI. #7003

Merged
merged 7 commits into from May 31, 2018

Conversation

Projects
None yet
6 participants
@jorgefilipecosta
Member

jorgefilipecosta commented May 29, 2018

Description

This PR extends the blocks API with two new selectors getChildBlocks and hasChildBlocks.
A class is added for inserter items with children and the UI is changed using the new class. Now we have a special design in blocks with children (this design is in progress).
A new area that appears when we are inserting blocks inside blocks with children was created.

*/
export const hasChildBlocks = createSelector(
( state, blockName ) => {
return getChildBlocks( state, blockName ).length > 0;

This comment has been minimized.

@gziolo

gziolo May 29, 2018

Member

This selector only wraps another selector. It shouldn’t use another layer of caching.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 29, 2018

Member

Nice catch @gziolo, I updated the code!

@mtias

This comment has been minimized.

Contributor

mtias commented May 29, 2018

Closes #6998.

@gziolo

This comment has been minimized.

Member

gziolo commented May 29, 2018

One of the unit tests is failing.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 29, 2018

Hi @gziolo the test case was fixed.
Thank @mtias for the design updates.

@mtias

This comment has been minimized.

Contributor

mtias commented May 30, 2018

Pushed some further tweaks to the design:

image

@jasmussen I think the biggest thing missing is addressing the blurry icons :)

@mtias mtias added this to the 3.0 milestone May 30, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

Nice work. I unblurred the icons :D

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 30, 2018

I rebased this PR and it seems to be working fine.

<div className="editor-inserter__child-blocks">
<div className="editor-inserter__parent-block-header">
<div className="editor-inserter__parent-block-icon">
<BlockIcon icon={ rootBlockIcon } />

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

BlockIcon should be rendered only when the icon is set

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 30, 2018

Member

Normally we never do this check because BlockIcon handles it but in this case, we can do it because we have wrapping div for the icon.

@@ -176,6 +184,18 @@ export class InserterMenu extends Component {
/>
<div className="editor-inserter__results">
{ !! childItems.length &&

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Can we extract this code to a new component to avoid confusing conditional checks for an icon and title? As far as I understand when there are child blocks then the icon and title must be set, right?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 30, 2018

Member

Yes if we have childblocks the icon and title are set unless the block did not use a title/item, but that's part of the block responsibility.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 30, 2018

Member

The child blocks rendering logic was extracted to a separate component.

*
* @return {Array} Array of child block names.
*/
export const getChildBlockNames = createSelector(

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

This selector seems to be quite important so let’s add unit tests to avoid regressions in the future.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 30, 2018

Member

Unit tests added 👍

const filteredItems = searchItems( items, filterValue );
const childItems = rootChildBlocks ?

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

rootChildBlocks is always an array, so in JS it’s always truthy.

Polish the paddings and search field.
This makes the search field a bit bigger and nicer, and changes the height to match better the contents.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 30, 2018

Pushed a little polish.

@@ -0,0 +1,29 @@
/**

This comment has been minimized.

@mtias

mtias May 30, 2018

Contributor

This could just be called ChildBlocks

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 30, 2018

Member

Renamed 👍

@gravityrail

LGTM! 🎉

@@ -176,6 +182,14 @@ export class InserterMenu extends Component {
/>
<div className="editor-inserter__results">
<ChildBlocks
rootBlockIcon={ rootBlockIcon }

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Did you consider to use withSelect directly on ChildBlocks component instead of passing down rootBlockIcon and rootBlockTitle?

import BlockIcon from '../block-icon';
function ChildBlocks( { rootBlockIcon, rootBlockTitle, items, ...props } ) {
if ( ! items || ! items.length ) {

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Can we use ifCondition HOC here?

@gziolo

Code changes look good, I left 2 more questions, but definitely not blockers. Nice work 👍

@gziolo

gziolo approved these changes May 30, 2018

* @param {string} blockName Block type name.
*
* @return {Array} Array of child block names.
*/

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

Not sure a JSDoc parser would detect this as being "attached" to the getChildBlockNames function with the newline between.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 31, 2018

Member

The line breaks were corrected.

'editor-inserter__item',
getBlockMenuDefaultClassName( item.id ),
{
'editor-inserter__item-with-childs': item.hasChildBlocks,

This comment has been minimized.

@aduth

aduth May 30, 2018

Member
  • Grammar: "childs" -> "children"
  • This is a modifier, so could be written as one, e.g. has-children (reference)
@@ -1397,6 +1397,7 @@ export const getInserterItems = createSelector(
isDisabled,
utility: calculateUtility( blockType.category, count, isContextual ),
frecency: calculateFrecency( time, count ),
hasChildBlocks: hasChildBlocks( blockType.name ),

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

How is the cache bust if we proceed to register a block defining blockType.name as a valid parent ?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 31, 2018

Member

We clean the cache each time state.blockTypes changes, so when we register a block the cache is invalidated.

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

We clean the cache each time state.blockTypes changes, so when we register a block the cache is invalidated.

But state.blockTypes (from core/blocks store) is not considered when we call getInserterItems (from core/editor store).

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 31, 2018

Member

I understand now the problem, getInserterItems is cached and depends on core/blocks store when core/blocks change the cache may not be updated. I think this is an already existing problem. Before if we register a new block a new item should exist but the cache may not be updated, it is not easy to spot because the cache updates a lot.

Thank you for clarifying this issue I will try to propose a solution to this problem!

@jorgefilipecosta jorgefilipecosta merged commit c93b778 into master May 31, 2018

2 checks passed

codecov/project 46.47% (+0.01%) compared to 02c9edb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/child-blocks-improvements branch May 31, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 31, 2018

Thank you @jasmussen and @mtias for the design improvements. Thank you @aduth, @gravityrail, and @gziolo for the reviews!

@afercia afercia referenced this pull request May 31, 2018

Closed

Inserter UI adjustments #7059

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 1, 2018

Great work, thank you.

I'm not sure this PR is the cause, but recently I've started seeing duplicate items in the slash command:

screen shot 2018-06-01 at 09 37 11

Simply search for "/image" and you'll see it. Ring any bells?

@aduth

This comment has been minimized.

Member

aduth commented Jun 6, 2018

@jasmussen Do you still see this on master? I'm not able to reproduce. Can we create a separate issue if it's still present?

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 6, 2018

Sorry, @jasmussen I missed the comment but like @aduth I'm not being able to reproduce the issue in master or in the last release 3.0.1. Are you still able to reproduce the problem?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 6, 2018

@aduth @jorgefilipecosta Can confirm that I'm not seeing this anymore. I don't know what fixed it, I could swear I saw it for /gallery as soon as yesterday. But I can't repro.

Edit: also seeing it for Cover Image now.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 7, 2018

@aduth @jorgefilipecosta I now see it in master again. I have zero idea why it reappeared. And I can't reproduce it for all blocks. I now see it for Heading and Gallery:

repeat

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