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

Spacer block: Add clear:both; to clear any floats around it #15874

Merged
merged 3 commits into from Jun 7, 2019

Conversation

@mapk
Copy link
Contributor

commented May 29, 2019

Fixes #15099. Adds a clear:both; to the Spacer block so that it clears any floated blocks and functions as a Spacer block should.

How has this been tested?

Locally.

Screenshots

Current result adding Spacer block under floated images

Screen Shot 2019-05-28 at 8 15 03 PM

Proposed result adding Spacer block that clears floats

Screen Shot 2019-05-28 at 8 03 37 PM

Types of changes

Added minor CSS. I tested under various use cases.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
Fixes #15099. Adds a to the Spacer block so that it clears any floate…
…d blocks and functions as a Spacer block should.
@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

This works well:

Before:

Screen Shot 2019-05-30 at 3 09 32 PM

After:

Screen Shot 2019-05-30 at 3 10 20 PM

I'm trying to think of a reasonable scenario when you wouldn't want this to clear floats, and I'm coming up empty. I'd love a gut check from @jasmussen though just to be sure.

If we do include this, I agree with @marekhrabe that we should provide a similar rule on the front end too. To do that, we'd need to add a new file at packages/block-library/src/spacer/style.scss, and add a style to clear .wp-block-spacer. Then just add it to the list of includes at packages/block-library/src/style.scss.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I'm trying to think of a reasonable scenario when you wouldn't want this to clear floats, and I'm coming up empty. I'd love a gut check from @jasmussen though just to be sure.

The one that comes to mind is the following (pseudo) markup:

  1. Image, floated right
  2. Text
  3. Spacer below text
  4. Text

And the spacer in 3 is inserted in order to "fill out" the space below the text in 2, so the text in 4 clears the floated image.

I would actually suggest that your "before" image, Kjell, is the desired behavior. The UI is just not very pretty — but it's actually representative of the markup below — because of the weirdness of hwo floats work. The block actually does span the full width, it's just the content of the block that's "floating around".

I do recognize what I'm describing above is the 1% use case, and is likely not what the user expects. So I have no objections to this PR shipping. But I can imagine a future where float clearing is a toggle in the "advanced" section of blocks, and it's then up to blocks themselves to decide whether they should clear by default or not.

@mapk

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

And the spacer in 3 is inserted in order to "fill out" the space below the text in 2, so the text in 4 clears the floated image.

Good scenario! And you're right, it does follow the structure of the markup this way, but as agreed, it's probably a 1% scenario that would want this. Based on that, I'm okay forcing the Spacer block to clear floats because I believe this would be the more common use case.

So I have no objections to this PR shipping.

Thanks, @jasmussen! I'll also work on the frontend stylesheet as mentioned by both @kjellr and @jorgefilipecosta.

Thanks everyone for this great feedback! 🥇

Oh, yeah, BTW, I love this thought!

But I can imagine a future where float clearing is a toggle in the "advanced" section of blocks, and it's then up to blocks themselves to decide whether they should clear by default or not.

@jorgefilipecosta
Copy link
Member

left a comment

Hi @mapk, what if we add the clear both style to the selector .block-editor-block-list__block[data-type="core/spacer"]?
It seems a more robust style that does not rely as much on the space block internals. There is a small issue with the approach I suggested, in my tests the spacer toolbar border is cut a bit. I guess we may debug the problem separately and even with this small visual issue the UX with that selector seems improved (in my opinion).

@mapk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

There is a small issue with the approach I suggested, in my tests the spacer toolbar border is cut a bit. I guess we may debug the problem separately and even with this small visual issue the UX with that selector seems improved (in my opinion).

@jorgefilipecosta I'm a bit unclear if you'd like me to make that change, or because you found some issues, to just leave it as is. Can you clarify?

I've also added a stylesheet for the frontend. Thanks for the guide there, @kjellr!

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@jorgefilipecosta I'm a bit unclear if you'd like me to make that change, or because you found some issues, to just leave it as is. Can you clarify?

Tying this to the core block itself, instead of its inner container would push the floated block outside of the entire spacer block. For instance, here's the current approach:

Screen Shot 2019-06-05 at 9 59 35 AM

... and here's what would happen if the floating was cleared at .block-editor-block-list__block[data-type="core/spacer"]:

Screen Shot 2019-06-05 at 9 59 49 AM

This technically feels a little better. It does have a small toolbar overlap issue, but that could be easily solved by setting the toolbar z-index to a value above 81. I'm not sure if that has other unintended consequences though.

Your current approach here is in line with the way floated blocks appear inside of blocks like the Paragraph block:

Screen Shot 2019-06-05 at 10 06 35 AM

So I think it's fine to keep the current approach for now.

@kjellr
Copy link
Contributor

left a comment

Left one very small CSS note. Once that's all set, I think we're good to go!

@@ -24,6 +24,7 @@
@import "./text-columns/style.scss";
@import "./verse/style.scss";
@import "./video/style.scss";
@import "./spacer/style.scss";

This comment has been minimized.

Copy link
@kjellr

kjellr Jun 5, 2019

Contributor

Really minor note, but this list is arranged alphabetically. Can you move this line up to the correct alphabetical spot?

This comment has been minimized.

Copy link
@mapk

mapk Jun 7, 2019

Author Contributor

Haha, totally! Thanks for catching that one.

@mapk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@kjellr or @jorgefilipecosta one last review please. 😄

@kjellr

kjellr approved these changes Jun 7, 2019

Copy link
Contributor

left a comment

🎉 Looks great, thanks for the fix, @mapk!

@kjellr kjellr merged commit 27cc263 into master Jun 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@kjellr kjellr added this to Done in Phase 2 via automation Jun 7, 2019

@kjellr kjellr deleted the update/spacer-block-clear-floats branch Jun 7, 2019

@kjellr kjellr added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Spacer block: Add clear:both; to clear any floats around it (WordPres…
…s#15874)

* Fixes WordPress#15099. Adds a  to the Spacer block so that it clears any floated blocks and functions as a Spacer block should.

* Minor lint-style fix.

* Moved the import file into alphabetical position.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.