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.

Sign up for GitHub

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

Improve media & text block on IE. #10812

Merged
merged 1 commit into from
Oct 11, 2019
Merged

Improve media & text block on IE. #10812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/packages/post-css-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@ module.exports = [
},
},
} ),
require( 'autoprefixer' ),
require( 'autoprefixer' )( { grid: true } ),
require( 'postcss-color-function' ),
];
4 changes: 3 additions & 1 deletion packages/block-library/src/media-text/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,10 @@ class MediaTextEdit extends Component {
'is-image-fill': imageFill,
} );
const widthString = `${ temporaryMediaWidth || mediaWidth }%`;
const gridTemplateColumns = 'right' === mediaPosition ? `1fr ${ widthString }` : `${ widthString } 1fr`;
const style = {
gridTemplateColumns: 'right' === mediaPosition ? `auto ${ widthString }` : `${ widthString } auto`,
gridTemplateColumns,
msGridColumns: gridTemplateColumns,
Copy link
Member

Choose a reason for hiding this comment

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

Note that style attributes must be whitelisted. This was missed for grid-template-columns previously, and can result in invalidated blocks for non-administrator users.

See also: https://core.trac.wordpress.org/ticket/46597

We should probably add a filter to include -ms-grid-columns for the safe_style_css set in the plugin, and propose a complementing upstream Trac ticket once this is merged.

https://developer.wordpress.org/reference/hooks/safe_style_css/

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @aduth, this PR is not changing the block save function/markup of the block. At the time (before 5.0 release) in talks with @mtias it was discussed if we should add an IE specific style to the block inline styles and we decided not to do it. The disadvantage is that on the website frontend the image always appears at 50% width, on the backend we add the correct style so it appears on the correct width.

Given that we are not adding -ms-grid-columns on the save function I don't think there is a need to update filters compared to what we are already doing on the patches available at https://core.trac.wordpress.org/ticket/46597.

Copy link
Member

Choose a reason for hiding this comment

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

this PR is not changing the block save function/markup of the block

Ah, you're right. Sorry, I missed that.

I guess it makes me wonder then what's the purported benefit, if we're only solving the issue in the editor, but not on the front-end. To me, it would seem the latter would be the more important anyways. At the very least, should it not be marked as fixing #11577 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the block does not work in IE (front-end and editor), the media element and the content don't event appear side by side. This PR fixes most of the problems on the front end (in the editor fixes all of them). The layout appears side by side the vertical-align options work, the background position change works, the only thing that does not work is the resizing on the front end (e.g: things appear always with the default 50% width), compared to the current state is a big improvement. Issue #11577 says "Media & Text block is not aligning in IE 11" and I guess that problem is fixed the alignments work as expected, maybe we can close and add a comment specifying that just the resizing functionality does not work on the front end.

backgroundColor: backgroundColor.color,
};
const colorSettings = [ {
Expand Down
73 changes: 59 additions & 14 deletions packages/block-library/src/media-text/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,32 @@
.wp-block-media-text {
grid-template-areas:
"media-text-media media-text-content"
"resizer resizer";
align-items: center;
.wp-block-media-text .__resizable_base__ {
grid-column: 1 / span 2;
grid-row: 2;
}

.wp-block-media-text.has-media-on-the-right {
grid-template-areas:
"media-text-content media-text-media"
"resizer resizer";
.wp-block-media-text.is-vertically-aligned-top {
.block-editor-inner-blocks,
.editor-media-container__resizer {
align-self: start;
}
}
.wp-block-media-text,
.wp-block-media-text.is-vertically-aligned-center {
.block-editor-inner-blocks,
.editor-media-container__resizer {
align-self: center;
}
}

.wp-block-media-text .__resizable_base__ {
grid-area: resizer;
.wp-block-media-text.is-vertically-aligned-bottom {
.block-editor-inner-blocks,
.editor-media-container__resizer {
align-self: end;
}
}

.wp-block-media-text .editor-media-container__resizer {
grid-area: media-text-media;
grid-column: 1;
grid-row: 1;
// The resizer sets a inline width but as we are using a grid layout,
// we set the width on container.
width: 100% !important;
Expand All @@ -27,13 +37,24 @@
height: 100% !important;
}

.wp-block-media-text .block-editor-inner-blocks {
.wp-block-media-text.has-media-on-the-right .editor-media-container__resizer {
grid-column: 2;
grid-row: 1;
}

.wp-block-media-text .editor-inner-blocks {
word-break: break-word;
grid-area: media-text-content;
grid-column: 2;
grid-row: 1;
text-align: initial;
padding: 0 8% 0 8%;
}

.wp-block-media-text.has-media-on-the-right .block-editor-inner-blocks {
grid-column: 1;
grid-row: 1;
}

.wp-block-media-text > .block-editor-inner-blocks > .block-editor-block-list__layout > .block-editor-block-list__block {
max-width: unset;
}
Expand Down Expand Up @@ -67,3 +88,27 @@ figure.block-library-media-text__media-container {
}
}
}

@media (max-width: #{ ($break-small) }) {
.wp-block-media-text.is-stacked-on-mobile {
.block-editor-inner-blocks {
grid-column: 1;
grid-row: 2;
}
.editor-media-container__resizer {
grid-column: 1;
grid-row: 1;
}
}
.wp-block-media-text.is-stacked-on-mobile.has-media-on-the-right {
.block-editor-inner-blocks {
grid-column: 1;
grid-row: 1;
}
.editor-media-container__resizer {
grid-column: 1;
grid-row: 2;
}
}
}

66 changes: 46 additions & 20 deletions packages/block-library/src/media-text/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,56 @@
.wp-block-media-text {
display: grid;
grid-template-rows: auto;
align-items: center;
grid-template-areas: "media-text-media media-text-content";
grid-template-columns: 50% auto;

&.has-media-on-the-right {
grid-template-areas: "media-text-content media-text-media";
grid-template-columns: auto 50%;
grid-template-columns: 50% 1fr;
.has-media-on-the-right {
grid-template-columns: 1fr 50%;
}
}

.wp-block-media-text.is-vertically-aligned-top {
align-items: start;
.wp-block-media-text__content,
.wp-block-media-text__media {
align-self: start;
}
}

.wp-block-media-text,
.wp-block-media-text.is-vertically-aligned-center {
align-items: center;
.wp-block-media-text__content,
.wp-block-media-text__media {
align-self: center;
}
}

.wp-block-media-text.is-vertically-aligned-bottom {
align-items: end;
.wp-block-media-text__content,
.wp-block-media-text__media {
align-self: end;
}
}

.wp-block-media-text .wp-block-media-text__media {
grid-area: media-text-media;
grid-column: 1;
grid-row: 1;
margin: 0;
}

.wp-block-media-text .wp-block-media-text__content {
grid-column: 2;
grid-row: 1;
word-break: break-word;
grid-area: media-text-content;
padding: 0 8% 0 8%;
}

.wp-block-media-text.has-media-on-the-right .wp-block-media-text__media {
grid-column: 2;
grid-row: 1;
}

.wp-block-media-text.has-media-on-the-right .wp-block-media-text__content {
grid-column: 1;
grid-row: 1;
}

.wp-block-media-text > figure > img,
.wp-block-media-text > figure > video {
max-width: unset;
Expand All @@ -58,7 +75,6 @@
clip: rect(0, 0, 0, 0);
border: 0;
}

/*
* Here we here not able to use a mobile first CSS approach.
* Custom widths are set using inline styles, and on mobile,
Expand All @@ -69,14 +85,24 @@
@media (max-width: #{ ($break-small) }) {
.wp-block-media-text.is-stacked-on-mobile {
grid-template-columns: 100% !important;
grid-template-areas:
"media-text-media"
"media-text-content";
.wp-block-media-text__media {
grid-column: 1;
grid-row: 1;
}
.wp-block-media-text__content {
grid-column: 1;
grid-row: 2;
}
}

.wp-block-media-text.is-stacked-on-mobile.has-media-on-the-right {
grid-template-areas:
"media-text-content"
"media-text-media";
.wp-block-media-text__media {
grid-column: 1;
grid-row: 2;
}
.wp-block-media-text__content {
grid-column: 1;
grid-row: 1;
}
}
}