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

<amp-experiment> 1.0 add style mutation support #24089

Merged
merged 2 commits into from Aug 20, 2019

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Aug 20, 2019

Add the following list of style property support

Support all values:
color
background-color
font-size
background-image
border-width
border-style
border-color

Support a list of values
visibility: hidden
display: none
position: static/relative/absolute/initial/inherit

Support mutation for NON AMP elements only
width and height (all values)

const SUPPORTED_STYLE_VALUE = {
'color': /#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3});?$/,
'color': /.*/,
Copy link
Contributor

Choose a reason for hiding this comment

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

might worth to extract a regex constant for this to save CPU on regex parsing and instantiation.
or maybe even set it to true and skip the regex matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I wanted to set the value to true and skip the regex matching. But there's tradeoff there, the code looks less cleaner. Ended up extracting the regex constant.

@zhouyx zhouyx merged commit 2ab77e5 into ampproject:master Aug 20, 2019
@zhouyx zhouyx deleted the experiment-regex branch August 20, 2019 23:35
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
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.

None yet

3 participants