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

Add functionality on inner blocks to not include parts of the UI. #18173

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Oct 29, 2019

Description

This PR adds functionality on InnerBlocks that allows disabling parts of the block UI.

Example:

				<InnerBlocks
					allowedBlocks={ ALLOWED_BLOCKS }
					renderAppender={ renderAppender }
					template={ BUTTONS_TEMPLATE }
					__experimentalUIParts={ {
						hasSelectedUI: false,
						hasFocusedUI: false,
						hasHoveredUI: false,
						hasBreadcrumbs: false,
						hasMovers: false,
						hasSpacing: false,
						hasSideInserter: false,
					} }
				/>

By default, all the UI parts are still enabled.

This functionality will be used by buttons block #17352. As a follow-up, we can also refactor social links block to use this functionality, allowing us to remove some CSS hacks the block currently uses.

How has this been tested?

I tested this PR on the buttons block PR #17352. I verified all the removed UI parts don't appear inside the buttons block, and that the editor still behaves as expected.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Oct 29, 2019

VERY excited for this one; can’t wait to try it!

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Oct 30, 2019

Any thoughts about inheritance? do we want cascade or not?

@simison

This comment has been minimized.

Copy link
Member

simison commented Oct 31, 2019

hasBreadcrumbs: false,
hasMovers: false,
hasSpacing: false,
hasSideInserter: false,

By default, all the UI parts are still enabled.

If someday a new UI pattern hasSomeExample is added to Gutenberg, it would be set to visible by default.

Would it be convenient to have hasUI or similar that turns always everything off by default? — or does setting all hasSelectedUI/hasFocusedUI/hasHoveredUI to false already ensure that?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 1, 2019

Any thoughts about inheritance? do we want cascade or not?

HI @youknowriad, that's an excellent question. My plan, for now, is not to add inheritance.
Imagining the following scenario:
Block (A) uses these flags to remove the UI of its nested blocks, inside we have a Block (B), the block (B) has a columns block inside.

Do we expect the columns block to be different just because it is nested inside the block (B)? I don't think so, I think columns block should look the same.

I expected these flags to be used in situations where parent and child are very connected ( buttons-> button, social icons -> social icon, navigation menu -> navigation item, gallery -> image), and for these cases, it seems inheritance is not needed.

If someday a new UI pattern hasSomeExample is added to Gutenberg, it would be set to visible by default.

Would it be convenient to have hasUI or similar that turns always everything off by default? — or does setting all hasSelectedUI/hasFocusedUI/hasHoveredUI to false already ensure that?

Yes, I guess a flag that is an alias for other flags makes sense. But for now, I would prefer to go this granular path. I think then we may remove granularity as maybe some flag combinations don't make sense. When we decide on the list of flags we will provide, if there are many, I guess some alias flags should be provided.

@jorgefilipecosta jorgefilipecosta force-pushed the add/functionality-on-innerblocks-to-disable-part-of-the-block-ui branch from f931bff to 9c9ae58 Nov 1, 2019
@youknowriad youknowriad requested review from aduth and mtias Nov 2, 2019
@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 2, 2019

Do we expect the columns block to be different just because it is nested inside the block (B)? I don't think so, I think columns block should look the same.

There's probably cases where you want the parent to force the experience, becoming kind of an editor provider in terms of the experience.

But for now, I would prefer to go this granular path.

It might be fine to let you return false on the whole attribute as a way to disable everything instead of an object. A separate hasUI flag seems it could be more confusing.

@@ -53,7 +53,7 @@

// Provide every block with a default base margin. This margin provides a consistent spacing
// between blocks in the editor.
.editor-styles-wrapper [data-block] {
.editor-styles-wrapper .block-editor-block-list__block.has-spacing > .block-editor-block-list__block-edit > [data-block] {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 3, 2019

Author Member

@jasmussen said:

Is this necessary? I suspect this will cause a lot of editor styles to break because it adds needless extra specificity to editor styles.

We want to only apply the space/margins for blocks in the area that should contain spacing. And this seems to be the way to select these cases. We may also add has-spacing classes to components bellow to avoid the need for a selector with specificity so hight. Do you think that would help?

I needed to decide between:

  • Add spaces to every block and have another rule (very complex using the not selector) that overwrites the spaces when they are not needed.
  • Add spaces only when they are needed.

I followed the approach of adding the spaces (margins and paddings) only when they are needed.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 4, 2019

Contributor

As a quick preface, many of these review items happened on a separate PR where I did not have the full context of what you're trying to do here. Now that I do have that context, I applaud you for taking on the task. It's difficult, and many review points need to be thought about in a different way, but I absolutely see the purpose and want to make myself available to help if I can. Notably around spacing I have a great deal of institutional knowledge that might help there.

So we do need to avoid specificity whenever possible. I'd like to try and zoom out a bit here.

  1. We do want to provide a baseline margin to most blocks.
  2. But not all, blocks should be able to opt out.

If that is the goal, can we do something like [data-has-spacing]? For #17267 I was thinking whether it might make sense to have a [data-block-container] as well, to help ambiguate.

The benefit of using these data attributes is that they are entirely optional, and they are added to the innermost div they can be, which means we can avoid the > selector which is very hard to override in CSS.

@include break-small() {
// Compensate for side UI width.
.block-editor-block-list__block-edit {
> .block-editor-block-list__block-edit {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 3, 2019

Author Member

@jasmussen said:

Not sure what this does, but it means all full-wide blocks now have margin left and right of them:

Nice catch It happened because of the specificity increase of .has-spacing. I think if we follow this path we need to test really well and do a style audit to catch these cases.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 4, 2019

Contributor

I would encourage us to think of increasing the style specificity as plan C. If there is anything at all we can do to avoid increasing the specificity, it would be much preferable, as this really has compounding effects.

}
}

.block-editor-block-list__layout .block-editor-block-list__block {
position: relative;
.block-editor-block-list__block .block-editor-block-list__layout.has-spacing {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 3, 2019

Author Member

@jasmussen said:

I definitely emphathise with the complex block paddings. But this PR seems to go a bit farther than necessary in making things work. Perhaps it's worth waiting for #17877 to land before rebasing this on top?

Alternately, and I suspect this is due to the onerous extra UI applied to child blocks, you could snatch the code I wrote for social links, that should allow you to change the margins and paddings just for this one block, while awaiting better improvements than adding a new has-spacing property. Specifically it starts here: /packages/block-library/src/social-links/editor.scss@master#L14

Try the social links block and see how child blocks are well spaced out. If you need my help, feel free to ping, and I'll take a look.

Hi @jasmussen, previously, this PR used precisely the same mechanism of the social link blocks. The reason why I updated this PR was to add a mechanism in the BlockList that allows every block to have a similar UI without the need to write this CSS.

I think the process of applying the margins spacing conditionally to blocks in some areas will always evolve more complex and specific CSS selectors.
I think the spacing flag is the more complex one, and given that a PR is in progress to refactor that part, it may be wise to revert the spacing changes and focus on the other flags for now. And then, after your PR is merged, we create a specific PR to focus on the spacing flag.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 4, 2019

Contributor

The reason why I updated this PR was to add a mechanism in the BlockList that allows every block to have a similar UI without the need to write this CSS.

Wonderful. I entirely missed that goal. It's a noble one.

I think the spacing flag is the more complex one, and given that a PR is in progress to refactor that part, it may be wise to revert the spacing changes and focus on the other flags for now. And then, after your PR is merged, we create a specific PR to focus on the spacing flag.

If I understand this PR correctly, it touches vertical spacing/margins, not horizontal ones. That means it shouldn't necessarily be that affected by #17877. However the rebase might still be gnarly, and given the wide images regressed maybe it will matter after all. In any case, #17877 is approved, I actually just need to know when is a good time to merge it because of the amount of red in the PR.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 23, 2019

Author Member

This PR was rebased and updated. The margin changes were removed. Some flags become unnecessary because of the latest refactors/design changes. The two flags remaining are the ones to disable movers and to disable the selection UI.

@jorgefilipecosta jorgefilipecosta force-pushed the add/functionality-on-innerblocks-to-disable-part-of-the-block-ui branch from 9c9ae58 to e5c31e8 Dec 23, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 2, 2020

I think this PR might become unnecessary entirely in the new design since the selected UI is removed and the movers are moved to the block toolbar, that said, I'm ok merging this as experimental to unblock the buttons block.

@jorgefilipecosta jorgefilipecosta merged commit cd01549 into master Jan 2, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the add/functionality-on-innerblocks-to-disable-part-of-the-block-ui branch Jan 3, 2020
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 14, 2020

@jorgefilipecosta I see this has been added to the group block's inner blocks, but why should they not have movers?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 14, 2020

Hi @ellatrix the issue was solved in #19640.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.