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

Style unresolved elements #12077

Merged
merged 5 commits into from Nov 17, 2017

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Nov 15, 2017

This applies initial layouts to still unresolved AMP elements.

The changes to layout=nodisplay, layout=responsive, layout=fixed-height, layout=container, layout=fill, and layout=flex-item just apply while the element is unresolved (doesn't have a .i-amphtml-layout-X class). Once it is resolved, the class will be added and selector will fail to apply, so everything keeps the same specificity as before.

The other changes to unresolved elements with any layout ([layout]:not(.i-amphtml-element)) prevent the children of unresolved elements from being displayed, since the parent needs to resolve before we can figure out the measurements for the children. This fixes #11929.

/cc @brandondiamond

@@ -144,31 +144,47 @@ html.i-amphtml-ios-embed.i-amphtml-ios-overscroll > #i-amphtml-wrapper {
display: inline-block;
}

.i-amphtml-layout-nodisplay {
.i-amphtml-layout-nodisplay,
[layout=nodisplay]:not(.i-amphtml-layout-nodisplay)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need the :not(.i-amphtml-layout-nodisplay) here? , i don't think it makes a difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once it is resolved, the selector will fail to apply, so everything keeps the same specificity as before.

css/amp.css Outdated
/* Display is set/reset in JS */
}

.i-amphtml-layout-fixed {
.i-amphtml-layout-fixed,
/*, [width][height]:not([layout=fixed]) */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: delete comment

css/amp.css Outdated
.i-amphtml-layout-fixed {
.i-amphtml-layout-fixed,
/*, [width][height]:not([layout=fixed]) */
[layout=fixed][width][height]:not(.i-amphtml-layout-fixed)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change (almost in all the places), since we are increasing the specificity here. It is probably going to break a lot of pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once it is resolved, the selector will fail to apply, so everything keeps the same specificity as before.

@jridgewell
Copy link
Contributor Author

/to @dvoytenko

Copy link
Contributor

@dvoytenko dvoytenko 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. LGTM with a couple of comments. PTAL and merge when ready.

css/amp.css Outdated
position: relative;
overflow: hidden !important;
color: transparent !important;
}

.i-amphtml-notbuilt:not(.i-amphtml-layout-container) > * {
.i-amphtml-notbuilt:not(.i-amphtml-layout-container) > * ,
[layout]:not(.i-amphtml-element):not(.i-amphtml-layout-container) > *,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps [layout]:not([layout="container"]):{the rest of what you have}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

position: relative;
overflow: hidden !important;
color: transparent !important;
}

.i-amphtml-notbuilt:not(.i-amphtml-layout-container) > * {
.i-amphtml-notbuilt:not(.i-amphtml-layout-container) > * ,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please double-check that the placeholder declaration below (.i-amphtml-element > [placeholder], etc) has higher specificity than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.i-amphtml-layout-responsive {
.i-amphtml-layout-responsive,
[layout=responsive][width][height]:not(.i-amphtml-layout-responsive),
[width][height][sizes]:not(.i-amphtml-layout-responsive)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider additional, [height][width=auto] - it implies layout=responsive.

@jridgewell jridgewell merged commit c5dc94c into ampproject:master Nov 17, 2017
@jridgewell jridgewell deleted the unresolved-layout-css branch November 17, 2017 23:28
ghost pushed a commit that referenced this pull request Dec 6, 2017
* Target unresolved amp elements with expected layout

* Apply layouts to unresolved elements

* Ensure placeholder are shown

* Remove comments

* Update layout unresolved selectors
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* Target unresolved amp elements with expected layout

* Apply layouts to unresolved elements

* Ensure placeholder are shown

* Remove comments

* Update layout unresolved selectors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amp-carousel causing document height to fluctuate / be too large
4 participants