Skip to content

/ gutenberg

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

Jump to bottom

Try: output float clearing for all centered blocks. #21608

Merged
merged 1 commit into from Apr 21, 2020
Merged

Try: output float clearing for all centered blocks. #21608

merged 1 commit into from Apr 21, 2020

Conversation

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Apr 15, 2020

This PR needs feedback. It is a replacement for #16780.

What it does is output a simple float-clearing rule for any block that is aligned center.

To test, insert two images. The first one you float left, the 2nd one you align center.

Before:

Screenshot 2020-04-15 at 13 46 21

After:

Screenshot 2020-04-15 at 13 46 28

To be perfectly clear, this is output in the editor and on the frontend.

In the past, this has been the purview of the theme itself, but lately there has been some discussion on how much or how little the editor should "help" here. That's why this PR needs discussion.

  • Either we do it, and helps #10299.
  • Or, we choose to not do it, and #10299 will be the de-facto solution to float clearing.
@jasmussen
Try: output float clearing for all centered blocks.
Loading status checks…
cb84875
@jasmussen jasmussen self-assigned this Apr 15, 2020
@jasmussen jasmussen mentioned this pull request Apr 15, 2020
Closed
5 of 5 tasks complete
@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Apr 15, 2020

I don't personally have a strong opinion here. It feels like CSS to support sort of an edgecase, which suggests we might not want to do it, and let themes handle it in the theme itself AND the editor. But at the same time, it does feel weird that a centered block following a floated block might look broken, and the fix is easy.

So I created this PR to move things forward. If we choose to not do this, as mentioned, the way to address this is a user option as outlined in #21608.
@jasmussen jasmussen mentioned this pull request Apr 15, 2020
Open
@github-actions
Copy link

@github-actions github-actions bot commented Apr 15, 2020

Size Change: +11 B (0%)

Total Size: 839 kB

Filename Size Change
build/block-library/style-rtl.css 7.14 kB +6 B (0%)
build/block-library/style.css 7.15 kB +5 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.11 kB 0 B
build/block-library/editor.css 7.11 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.6 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 27.8 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 4.65 kB 0 B
build/edit-widgets/style.css 4.64 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action
@ZebulanStanphill
ZebulanStanphill approved these changes Apr 15, 2020
View changes
Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

I'm not usually a fan of opinionated styles, but I can't think of any practical downsides to this. It seems reasonable enough to me.
@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Apr 16, 2020

Thank you Zeb! I'm going to actually leave this up for a while longer just to gather some more thoughts.
@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Apr 21, 2020

Given there's been no feedback, through the law of diminishing returns and the fact that it's trivial to revert, I'm going to merge this one.
@jasmussen jasmussen merged commit 4e1320c into master Apr 21, 2020
4 checks passed
4 checks passed
build
Details
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the try/clear-centered-floats branch Apr 21, 2020
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 21, 2020
@jasmussen jasmussen mentioned this pull request Apr 24, 2020
Merged
@inc2734 inc2734 mentioned this pull request May 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZebulanStanphill ZebulanStanphill

@talldan talldan

@Soean Soean

@ajitbohra ajitbohra

@jorgefilipecosta jorgefilipecosta

Assignees

@jasmussen jasmussen

Labels
Needs Decision Needs Design Feedback [Feature] Themes
Projects
None yet
Milestone
Gutenberg 8.0
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@jasmussen @ZebulanStanphill
You can’t perform that action at this time.