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

Tree shaker removes CSS style rules for amp-date-picker classes #4334

Closed
westonruter opened this issue Feb 25, 2020 · 8 comments · Fixed by #4339
Closed

Tree shaker removes CSS style rules for amp-date-picker classes #4334

westonruter opened this issue Feb 25, 2020 · 8 comments · Fixed by #4339
Assignees
Labels
Bug Something isn't working CSS
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

As reported in the support forum, styling the amp-date-picker component is difficult because style rules that target classes which the component adds via JS are removed by the tree-shaker. For example:

amp-date-picker .CalendarMonth_caption {  
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
amp-date-picker .amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
.amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;	
}

A workaround is to ensure that all CSS selectors have an amp-date-picker ancestor prefix, and then to add this plugin code:

// Temporary workaround for amp-date-picker class names having CSS selectors being tree-shaken.
add_filter( 'amp_content_sanitizers',
	function ( $sanitizers ) {
		$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] = array_merge(
			! empty( $sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] ) ? $sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] : [],
			[
				// Modified from from protected AMP_Style_Sanitizer::$DEFAULT_ARGS.
				'amp-list',
				'amp-live-list',
				'[submit-error]',
				'[submit-success]',
				'amp-script',

				// New.
				'amp-date-picker',
			]
		);

		return $sanitizers;
	}
);

Expected Behaviour

If an amp-date-picker is on the page, all of the class names that can be used in an amp-date-picker should be considered exempt from tree-shaking.

Steps to reproduce

Create a Custom HTML block with the following contents:

<style>
amp-date-picker .CalendarMonth_caption {  
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
amp-date-picker .amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
.amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;	
}
</style>
<amp-date-picker layout="fixed-height" height="360"></amp-date-picker>

View the AMP version of the page and notice that the date picker header does not have the expected gradient.

Screenshots

Expected:

image

Actual:

image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Bug Something isn't working CSS labels Feb 25, 2020
@westonruter westonruter added this to the v1.5 milestone Feb 25, 2020
@westonruter

This comment has been minimized.

@kienstra kienstra self-assigned this Feb 26, 2020
@kienstra
Copy link
Contributor

kienstra commented Feb 26, 2020

Approach

Hi @westonruter,
No hurry responding, you're traveling and working 😄

But when you have a chance, what do you think about:

  1. Prevent tree-shaking of any selectors starting with amp-date-picker, like the example you gave of amp-date-picker .CalendarMonth_caption

It looks like the amp-date-picker documentation encourages styling many classes:

In order to personalize amp-date-picker style, you can use classes such as CalendarMonth_caption which are inherited from react-dates.

And the component in react-dates looks to have a lot of classes, probably more than we could keep track of in a PHP file. They're the PascalCase classes:

react-co

  1. But if the selector has i-amphtml-* classes somewhere, like amp-date-picker .i-amphtml-date-picker-container, strip them. They're of course invalid AMP.

Maybe this would be applied in:

// @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags.

Thanks, Weston!

@westonruter
Copy link
Member Author

  1. Prevent tree-shaking of any selectors starting with amp-date-picker, like the example you gave of amp-date-picker .CalendarMonth_caption

This would be only a partial solution. We can support all the class names.

And the component in react-dates looks to have a lot of classes, probably more than we could keep track of in a PHP file.

We wouldn't need to keep track of every one. We'd just need to keep track of the ones that have a common prefix corresponding to the component names seen here: https://github.com/airbnb/react-dates/tree/master/src/components

For example, we only need to see if a class name starts with CalendarMonth to determine it is for amp-date-picker.

  1. But if the selector has i-amphtml-* classes somewhere, like amp-date-picker .i-amphtml-date-picker-container, strip them. They're of course invalid AMP.

This wouldn't need to be addressed in this issue.

@kienstra
Copy link
Contributor

Nice, thanks for looking at this, and for your really fast reply.

@kienstra
Copy link
Contributor

kienstra commented Mar 3, 2020

This is probably a small.

@westonruter
Copy link
Member Author

Build for testing: amp.zip (v1.5.0-alpha-20200316T170457Z-c6b132818)

@westonruter
Copy link
Member Author

QA verified: https://wordpress.org/support/topic/amp-datepicker-unable-to-appy-custom-style-css/#post-12549873

@kienstra
Copy link
Contributor

Nice, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working CSS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants