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

#2504 Fix for aligned embed videos #3636

Merged
merged 1 commit into from Nov 28, 2017

Conversation

Projects
None yet
3 participants
@Mathiu
Contributor

Mathiu commented Nov 23, 2017

Description

Added static width on aligned video embeds, otherwise they have no width. Also made sure it works on mobile.

How Has This Been Tested?

Tested on PC Chrome 62 & Firefox 58 (beta5)

Types of changes

CSS

Checklist:

  • My code is tested (partially i guess?).
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
.editor-block-list__block[data-type="core/embed"] {
&[data-align="left"],
&[data-align="right"] {
max-width: 100%;

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Is this necessary? Or at least, should it be applying so specifically to the embed block type? Why aren't other aligned blocks susceptible to this?

// Alignments
&[data-align="left"],
&[data-align="right"] {
// Without z-index, won't be clickable as "above" adjacent content
z-index: z-index( '.editor-block-list__block {core/image aligned left or right}' );
max-width: 370px;
}

In my testing, it seems to work fine without this specific style.

@aduth

aduth Nov 27, 2017

Member

Is this necessary? Or at least, should it be applying so specifically to the embed block type? Why aren't other aligned blocks susceptible to this?

// Alignments
&[data-align="left"],
&[data-align="right"] {
// Without z-index, won't be clickable as "above" adjacent content
z-index: z-index( '.editor-block-list__block {core/image aligned left or right}' );
max-width: 370px;
}

In my testing, it seems to work fine without this specific style.

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Actually, from what I can tell there's an issue of specificity where the max-width: 700px of .editor-visual-editor .editor-block-list__block takes precedence over the max-width: 370px of .editor-block-list__block[data-align="left"]

.editor-visual-editor .editor-block-list__block {
margin-left: auto;
margin-right: auto;
max-width: $visual-editor-max-width + ( 2 * $block-mover-padding-visible );
&[data-align="wide"] {
max-width: 1100px;
}
&[data-align="full"] {
max-width: 100%;
}
&[data-align="full"],
&[data-align="wide"] {
.editor-block-contextual-toolbar {
@include break-small() {
width: $visual-editor-max-width + 2; // 1px border left and right
}
margin-left: auto;
margin-right: auto;
}
}
}

Might have been a regression accidentally introduced with changes of #3546.

@aduth

aduth Nov 27, 2017

Member

Actually, from what I can tell there's an issue of specificity where the max-width: 700px of .editor-visual-editor .editor-block-list__block takes precedence over the max-width: 370px of .editor-block-list__block[data-align="left"]

.editor-visual-editor .editor-block-list__block {
margin-left: auto;
margin-right: auto;
max-width: $visual-editor-max-width + ( 2 * $block-mover-padding-visible );
&[data-align="wide"] {
max-width: 1100px;
}
&[data-align="full"] {
max-width: 100%;
}
&[data-align="full"],
&[data-align="wide"] {
.editor-block-contextual-toolbar {
@include break-small() {
width: $visual-editor-max-width + 2; // 1px border left and right
}
margin-left: auto;
margin-right: auto;
}
}
}

Might have been a regression accidentally introduced with changes of #3546.

This comment has been minimized.

@Mathiu

Mathiu Nov 27, 2017

Contributor

It's only necessary to make aligned embed responsive. It's visible only at < 320px, I'm not sure if it should be supported.

Regarding specificity issue, the max-width is not related to fixing aligned embed videos, so it's probably unnecessary.

@Mathiu

Mathiu Nov 27, 2017

Contributor

It's only necessary to make aligned embed responsive. It's visible only at < 320px, I'm not sure if it should be supported.

Regarding specificity issue, the max-width is not related to fixing aligned embed videos, so it's probably unnecessary.

@aduth

aduth approved these changes Nov 28, 2017

@aduth aduth merged commit 086931d into WordPress:master Nov 28, 2017

2 checks passed

codecov/project 36.95% remains the same compared to 2056a3c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment