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

Fix: Don't show stacked icon on parent blocks if all of its children are hidden from the inserter #9337

Merged
merged 1 commit into from Sep 14, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Aug 24, 2018

Until now even if all the children of a block were hidden from the inserter we showed the stacked icon in the parent.
Given that for this cases no child blocks will appear on the inserter of the parent we should not show the stacked icon.
This fix is important for the hidden blocks feature and this PR was partially extracted from there #9259.

We also move the logic of getBlockSupport to a selector so other selectors can use it. Some of the other simpler functions available in the block registration API are also promoted to selectors so we can add the new selector needed that uses them.

How has this been tested?

I used the following test block https://gist.github.com/jorgefilipecosta/7dbcf8ce79c57acc6eb2c64f0088f480 and checked that the parent block does not contain a stacked icon.

@jorgefilipecosta jorgefilipecosta changed the title from Don't show stacked icon on parent blocks if all of its children are hidden from the inserter to Fix: Don't show stacked icon on parent blocks if all of its children are hidden from the inserter Aug 24, 2018

@mtias mtias added this to the 3.8 milestone Aug 31, 2018

@youknowriad youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

I guess I'm a bit confused / conflicted on what the stacking icon is meant to convey. To me, I had understood it to mean that there are additional blocks which can be inserted only within the context of this top-level block. That, and/or simply that it supports block nesting. For the Columns block, both are true: It supports nesting, and specifically nesting of blocks only supported within that type (the "Column" block), regardless whether those block types are surfaced via an inserter present.

Member

aduth commented Sep 13, 2018

I guess I'm a bit confused / conflicted on what the stacking icon is meant to convey. To me, I had understood it to mean that there are additional blocks which can be inserted only within the context of this top-level block. That, and/or simply that it supports block nesting. For the Columns block, both are true: It supports nesting, and specifically nesting of blocks only supported within that type (the "Column" block), regardless whether those block types are surfaced via an inserter present.

Show outdated Hide outdated packages/blocks/src/store/selectors.js Outdated
Show outdated Hide outdated packages/blocks/src/store/selectors.js Outdated
Show outdated Hide outdated packages/blocks/src/store/selectors.js Outdated
Show outdated Hide outdated packages/blocks/src/store/selectors.js Outdated
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 13, 2018

Member

Hi @aduth, thank you for the review the feedback was applied.

I guess I'm a bit confused / conflicted on what the stacking icon is meant to convey. To me, I had understood it to mean that there are additional blocks which can be inserted only within the context of this top-level block. That, and/or simply that it supports block nesting.

I think the meaning we want for the icon is to indicate to the user that inside that block (item on the inserter) there will be more inserter items available. On the columns case that's not the case, nothing exclusive to columns will appear on the inserter so that's the reason we are thinking about removing the icon from the columns (and similar external blocks).

Member

jorgefilipecosta commented Sep 13, 2018

Hi @aduth, thank you for the review the feedback was applied.

I guess I'm a bit confused / conflicted on what the stacking icon is meant to convey. To me, I had understood it to mean that there are additional blocks which can be inserted only within the context of this top-level block. That, and/or simply that it supports block nesting.

I think the meaning we want for the icon is to indicate to the user that inside that block (item on the inserter) there will be more inserter items available. On the columns case that's not the case, nothing exclusive to columns will appear on the inserter so that's the reason we are thinking about removing the icon from the columns (and similar external blocks).

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

If I'm of the minority opinion, I'm fine to relent and move forward with this change.

Member

aduth commented Sep 13, 2018

If I'm of the minority opinion, I'm fine to relent and move forward with this change.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

Am I missing something, or should I not expect to see the stacking effect here?

image

Member

aduth commented Sep 13, 2018

Am I missing something, or should I not expect to see the stacking effect here?

image

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 13, 2018

Member

Hi @aduth this PR does not change the columns it just changes the mechanism so a change to the columns can be applied. It can be tested using this block https://gist.github.com/jorgefilipecosta/7dbcf8ce79c57acc6eb2c64f0088f480.

The PR that applies a very trivial change to the column block (disables the inserter) is this one #9523. Probably I should have not divided the PR's given that it was a very simple change.

Member

jorgefilipecosta commented Sep 13, 2018

Hi @aduth this PR does not change the columns it just changes the mechanism so a change to the columns can be applied. It can be tested using this block https://gist.github.com/jorgefilipecosta/7dbcf8ce79c57acc6eb2c64f0088f480.

The PR that applies a very trivial change to the column block (disables the inserter) is this one #9523. Probably I should have not divided the PR's given that it was a very simple change.

@aduth

aduth approved these changes Sep 14, 2018 edited

Should we have the other pull request to be in the 3.9 milestone then?

@jorgefilipecosta jorgefilipecosta merged commit cd167ce into master Sep 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias deleted the fix/dont-show-child-stacked-icon-if-childs-are-hidden-on-inserter branch Sep 14, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

Missed running docs update. Local changes on master after running npm run docs:build

Member

aduth commented Sep 14, 2018

Missed running docs update. Local changes on master after running npm run docs:build

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