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

Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes #1111

Merged
merged 4 commits into from
May 3, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 3, 2018

This is a follow-up on #1048, and in particular this #1048 (comment) from @camelburrito:

usually the [class names] associated with amp-bind can be searchable in the dom (string search), even that would be a little risky because there could be string concat etc happening

I suspect that dynamic class names (constructed via concatenation) would be less likely than static ones, since dynamic class names would depend on having a correspondingly dynamic stylesheet. This doesn't seem likely.

With the changes in this PR, with the following markup:

<amp-state id="mySidebar">
	<script type="application/json">
		{"expanded": false}
	</script>
</amp-state>
<aside class="sidebar" [class]="mySidebar.expanded ? 'expanded' : ''">...</aside>

And given the following CSS:

.sidebar { display:none }
.sidebar.expanded { display:block }

The second style rule will no longer be removed when tree shaking is being performed.

This should greatly reduce the amount of times that someone has to manually inform style sanitizer of the dynamic_element_selectors, for example:

add_filter( 'amp_content_sanitizers', function( $sanitizers ) {
	if ( ! isset( $sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] ) ) {
		$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] = array();
	}
	$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] = array_merge(
		$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'],
		array(
			'amp-list',
			'amp-live-list',
			'[submit-error]',
			'[submit-success]',
			'.expanded', // <= Added.
			'.sidebar-open', // <= Added.
		)
	);
	return $sanitizers;
} );

@westonruter westonruter added this to the v1.0 milestone May 3, 2018
@westonruter westonruter requested a review from amedina May 3, 2018 05:45
@westonruter westonruter changed the title Prevent CSS tree shaking from removing selectors with classes mentioned in [class] attrs Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes May 3, 2018
@westonruter westonruter force-pushed the update/dynamic-element-selectors branch from e90f5c3 to 8cc3fb7 Compare May 3, 2018 16:04
@westonruter westonruter requested a review from kienstra May 3, 2018 19:21
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
Thanks, this looks good and works as expected. I also saw the issue of the .sidebar.expanded selector being stripped in the develop branch.

But with this PR, it appears as expected.

Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

LGTM

@camelburrito
Copy link

Thank you!! This looks great!

@westonruter westonruter merged commit 857e403 into develop May 3, 2018
@westonruter westonruter deleted the update/dynamic-element-selectors branch May 3, 2018 23:00
@postphotos postphotos added this to Definition in v1.0 May 22, 2018
@westonruter westonruter moved this from Definition to Ready for QA in v1.0 May 22, 2018
@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 6, 2018
@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v1.0
In Production
Development

Successfully merging this pull request may close these issues.

None yet

5 participants