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

Fixes overlaping column contents for embeds #21570

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

draganescu
Copy link
Contributor

Description

Closes #12283

I don't know why the min-width was 360px but setting it to 100% seems to both fix this and not create other issues.

How has this been tested?

Tested locally by:

  1. Add a post
  2. Add a columns block with 2 colums
  3. In one column add text and in the other add a YouTube embed
  4. See that the text is no longer overlapping with the embed

Screenshots

Screenshot 2020-04-14 at 11 37 50

Types of changes

Non breaking (from testing) CSS update for the embed block's editor CSS.

I don't know why the min-width was 360px but setting it to 100% seems to both fix this and not create other issues.
@github-actions
Copy link

github-actions bot commented Apr 14, 2020

Size Change: +6 B (0%)

Total Size: 839 kB

Filename Size Change
build/block-editor/index.js 104 kB -30 B (0%)
build/block-library/editor-rtl.css 7.1 kB -17 B (0%)
build/block-library/editor.css 7.09 kB -20 B (0%)
build/block-library/style-rtl.css 7.14 kB -10 B (0%)
build/block-library/style.css 7.15 kB -10 B (0%)
build/components/index.js 198 kB +14 B (0%)
build/editor/index.js 43.6 kB +56 B (0%)
build/format-library/index.js 7.32 kB +23 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/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 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/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/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/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.49 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

@jasmussen
Copy link
Contributor

Aside from the comment about the width I left, this does seem like the right approach. To elaborate, the min-width was added because some embeds aren't fully responsive, and that is out of our control as the oembed code and responsiveness is the prerogative of each embed provider. For example, some amazon embeds really don't work below a certain slot.

However instead of dumbing down the embed block per the lowest common denominator, we should remove that rule and let people open individual tickets on a per-embed basis.

@draganescu
Copy link
Contributor Author

@jasmussen could you code review and approve this as well if everything is all right? :)

@jasmussen
Copy link
Contributor

ON IT!

@jasmussen jasmussen self-requested a review April 15, 2020 08:20
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

So as it turns out, the rule you removed WAS appropriate to remove. But that a separate regression had happened.

Embeds that are responsive have no intrinsic dimensions. Which means if they are floated, they collapse to zero. So we do actually have to set either a min-width, or a width: 100% + max-width, when they are floated.

The CSS rules for handling that already existed in style.css, but had gotten out of date with recent float refactors in the editor, so I fixed that up.

Approving because this is good, but please sanity check the CSS and test it with a floated embed. Here's what I see:

Screenshot 2020-04-15 at 10 20 16

@draganescu
Copy link
Contributor Author

I retested with a floated embed as well and everything appears to look OK!

@draganescu draganescu merged commit 4924eff into master Apr 15, 2020
@draganescu draganescu deleted the fix/video-overflow-column branch April 15, 2020 14:13
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed video into column - text overlap
2 participants