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

Add Dynamic CSS Classes extension #1253

Merged
merged 1 commit into from
Dec 29, 2015

Conversation

jridgewell
Copy link
Contributor

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-class element that'll take a DSL like our event handlers:

<amp-dynamic-class group-a="group-a-name" group-b="group-b-name" on="expression > .10" />

But that'll need to be done at a later time.

referrer = parseUrl(referrer).hostname;
// Split all dots except for the last one.
const domains = referrer.split(/\.(?=.+\.)/);
let last;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this being used for? the name is not very clear.
prevDomain might be a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

* Returns an array of referrers which vary in level of subdomain specificity.
*
* @param {string} referrer
* @return {Array<string>}
Copy link
Contributor

Choose a reason for hiding this comment

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

!Array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 jridgewell force-pushed the dynamic-css-classes branch 4 times, most recently from e7bcae6 to 23d7f6d Compare December 29, 2015 18:41
import {log} from '../../../src/log';
import {isExperimentOn} from '../../../src/experiments';

/** @const **/
Copy link
Member

Choose a reason for hiding this comment

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

just minor nit for consistency, should just be /** @const */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jridgewell jridgewell force-pushed the dynamic-css-classes branch 2 times, most recently from 8ccf892 to 1a114d3 Compare December 29, 2015 18:49
The AMP Dynamic CSS Classes extension adds the following CSS classes
onto the HTML element:

**amp-referrer-***
Copy link
Member

Choose a reason for hiding this comment

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

trailing *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional, a bold amp-referrer-*.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha! 😸

@dvoytenko
Copy link
Contributor

LGTM on my side.

@dvoytenko
Copy link
Contributor

Just one comment on docs.

@dvoytenko
Copy link
Contributor

Looks like, though, it needs rebasing.

@jridgewell jridgewell force-pushed the dynamic-css-classes branch 2 times, most recently from eb6c989 to db50a4a Compare December 29, 2015 20:35
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.
@cramforce
Copy link
Member

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.

@jridgewell
Copy link
Contributor Author

Are you planning to ask user agent based classes in a follow up. If yes, that is a good idea

I was saving that for later. I just need a list of UAs we want to detect.

jridgewell added a commit that referenced this pull request Dec 29, 2015
@jridgewell jridgewell merged commit 3a28d82 into ampproject:master Dec 29, 2015
@jridgewell jridgewell deleted the dynamic-css-classes branch December 29, 2015 21:05
@jridgewell
Copy link
Contributor Author

Whoot, first merge. 🎆 🎊

@camelburrito
Copy link
Contributor

Congrats!

@dvoytenko
Copy link
Contributor

w00t!

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

Successfully merging this pull request may close these issues.

5 participants