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

Restore floated image margins. #21500

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Restore floated image margins. #21500

merged 1 commit into from
Apr 22, 2020

Conversation

jasmussen
Copy link
Contributor

The image block regressed in a recent refactor of left/right block margins. The margins for floated images were removed:

Screenshot 2020-04-09 at 11 31 25

This PR restores them. Editor:

Screenshot 2020-04-09 at 11 36 11

Frontend:

Screenshot 2020-04-09 at 11 36 17

Additionally, it adds a top and bottom margin, which was also present in the classic editor:

	.alignright {
		/*rtl:ignore*/
		float: right;
		/*rtl:ignore*/
		margin: 0.5em 0 0.5em 1em;
	}

	.alignleft {
		/*rtl:ignore*/
		float: left;
		/*rtl:ignore*/
		margin: 0.5em 1em 0.5em 0;
	}

But what about the Cover block, and other blocks that have alignments?

Those receive margins through this CSS:

.block-editor-block-list__layout .block-editor-block-list__block[data-align="left"] > .is-block-content {
    float: left;
    margin-right: 2em;
}

I'm open to advice on a more generic approach to this, but it seems worth fixing.

@jasmussen jasmussen self-assigned this Apr 9, 2020
@jasmussen jasmussen added [Block] Image Affects the Image Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended labels Apr 9, 2020
@jasmussen
Copy link
Contributor Author

@ellatrix if you have time, I would love your input, thank you!

@github-actions
Copy link

github-actions bot commented Apr 9, 2020

Size Change: +74 B (0%)

Total Size: 904 kB

Filename Size Change
build/block-library/editor-rtl.css 7.26 kB +30 B (0%)
build/block-library/editor.css 7.26 kB +30 B (0%)
build/block-library/style-rtl.css 7.43 kB +7 B (0%)
build/block-library/style.css 7.44 kB +7 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 104 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/index.js 112 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 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.5 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.37 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 93.5 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 3.74 kB 0 B
build/edit-widgets/style.css 3.73 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.6 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.29 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 711 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.5 kB 0 B
build/priority-queue/index.js 789 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

@@ -21,6 +21,19 @@
margin-top: -9px;
margin-left: -9px;
}

// These need specificity to override an inherited left/right block margin in the editor.
&.wp-block-image.alignleft {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be generally applied to any block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably. There's a generic float rule, as mentioned, that looks like this:

.block-editor-block-list__layout .block-editor-block-list__block[data-align="left"] > .is-block-content {
    float: left;
    margin-right: 2em;
}

But of course that only applies to blocks that have the "is-block-content" container. I can't recall the heuristic for when such a container.

@ellatrix
Copy link
Member

ellatrix commented Apr 9, 2020

Would also love @youknowriad's feedback since we've both worked on this. :)

@youknowriad
Copy link
Contributor

So this is just for the editor as these styles are already present on the frontend right? I don't mind the change but I'm looking forward the alignments refactoring where we'd remove all these hacks.

@jasmussen
Copy link
Contributor Author

So this is just for the editor as these styles are already present on the frontend right? I don't mind the change but I'm looking forward the alignments refactoring where we'd remove all these hacks.

This is if the theme doesn't provide an editor style, yes.

And this is the CSS it overrides:

Screenshot 2020-04-09 at 14 07 07

Will the alignments refactoring let us get rid of that rule?

@youknowriad
Copy link
Contributor

Will the alignments refactoring let us get rid of that rule?

I believe so yes, this rule shouldn't apply to floated blocks

@jasmussen
Copy link
Contributor Author

Cool. So should this PR land as is, or get design/themer feedback, or does it need to be refactored to be more generic to every floated block?

@youknowriad
Copy link
Contributor

I don't have a strong opinion, I'm fine with it landing, It's definitely better if it's applied to every floated blocks but I also think that if we find a good path forward for the alignments refactoring, everyone would gain from it (themes won't have to write specific styles, all blocks work similarly..., frontend and backend work similarly), the downside is that I don't see a path forward with a minimum of breaking changes.

@jasmussen
Copy link
Contributor Author

I'm happy to have this refactored away, and work as a band-aid in the mean time, if someone gives the thumbs up.

Themes increasingly provide these margins themselves, and I have verified that themes that do provide this still override this baseline. Here's from TwentyTwenty which provides that margin in rems:

Screenshot 2020-04-09 at 14 37 08

So this PR should be safe.

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label Apr 10, 2020
@karmatosed karmatosed self-requested a review April 22, 2020 13:25
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me from a design review perspective.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Apr 22, 2020
@jasmussen jasmussen merged commit f34d799 into master Apr 22, 2020
@jasmussen jasmussen deleted the try/float-margins branch April 22, 2020 13:46
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants