Add Dynamic CSS Classes extension #1253
Conversation
jridgewell
force-pushed
the
dynamic-css-classes
branch
from
5635f51
to
e7a5e1d
Compare
| referrer = parseUrl(referrer).hostname; | ||
| // Split all dots except for the last one. | ||
| const domains = referrer.split(/\.(?=.+\.)/); | ||
| let last; |
There was a problem hiding this comment.
what is this being used for? the name is not very clear.
prevDomain might be a better name
There was a problem hiding this comment.
It acts as a base domain so that www and google.com get concated as www.google.com, then the next will be concated with www.google.com, etc.
jridgewell
force-pushed
the
dynamic-css-classes
branch
from
e7a5e1d
to
c4ed477
Compare
| * Returns an array of referrers which vary in level of subdomain specificity. | ||
| * | ||
| * @param {string} referrer | ||
| * @return {Array<string>} |
jridgewell
force-pushed
the
dynamic-css-classes
branch
from
c4ed477
to
317b1c6
Compare
| <title>AMP #0</title> | ||
| <link rel="canonical" href="amps.html" > | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <script async custom-element="amp-dynamic-css-classes" src="/base/dist/v0/amp-dynamic-css-classes-0.1.max.js"></script> |
There was a problem hiding this comment.
So, this way the custom element itself is not involved, right? We need to review this since our plan was to eventually strip any custom-element script that has no actual elements in the document.
There was a problem hiding this comment.
There's no element included yet, but the plan was for eventual A/B testing.
@cramforce wanted this to be in an extension and not in core AMP ("it has too much already").
jridgewell
force-pushed
the
dynamic-css-classes
branch
4 times, most recently
from
e7bcae6
to
23d7f6d
Compare
| import {log} from '../../../src/log'; | ||
| import {isExperimentOn} from '../../../src/experiments'; | ||
|
|
||
| /** @const **/ |
There was a problem hiding this comment.
just minor nit for consistency, should just be /** @const */
jridgewell
force-pushed
the
dynamic-css-classes
branch
2 times, most recently
from
8ccf892
to
1a114d3
Compare
| The AMP Dynamic CSS Classes extension adds the following CSS classes | ||
| onto the HTML element: | ||
|
|
||
| **amp-referrer-*** |
There was a problem hiding this comment.
It's intentional, a bold amp-referrer-*.
|
LGTM on my side. |
|
Just one comment on docs. |
jridgewell
force-pushed
the
dynamic-css-classes
branch
from
1a114d3
to
2b59436
Compare
|
Looks like, though, it needs rebasing. |
jridgewell
force-pushed
the
dynamic-css-classes
branch
2 times, most recently
from
eb6c989
to
db50a4a
Compare
This extension adds the current referrer's domains as classes to the HTML element. If the current document is inside a viewer, it'll add a viewer CSS class ass well.
jridgewell
force-pushed
the
dynamic-css-classes
branch
from
db50a4a
to
8b46ced
Compare
|
Just wondering: Are you planning to ask user agent based classes in a follow up. If yes, that is a good idea :) Basically it would be a very small hard coded list of important user agents (Twitter, Facebook, Pinterest web view browser detection. Maybe a few more. |
I was saving that for later. I just need a list of UAs we want to detect. |
Add Dynamic CSS Classes extension
|
Whoot, first merge. 🎆 🎊 |
|
Congrats! |
|
w00t! |
This extension adds the current referrer's domains as classes to the HTML element. If the current document is inside a viewer, it'll add a viewer CSS class ass well.
Fixes #945.
For the A/B testing, I figure we can add an
amp-dynamic-css-classelement that'll take a DSL like our event handlers:But that'll need to be done at a later time.