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-sidebar - Whitelist checks and Documentation. #2812

Merged
merged 1 commit into from
Apr 8, 2016

Conversation

camelburrito
Copy link
Contributor

No description provided.

@@ -16,8 +16,9 @@

import {CSS} from '../../../build/amp-sidebar-0.1.css';
import {Layout} from '../../../src/layout';
import {isExperimentOn} from '../../../src/experiments';
import {assert} from '../../../src/asserts';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please no longer use assers.js - user user from log.js instead.

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

@camelburrito
Copy link
Contributor Author

PTAL

@camelburrito camelburrito force-pushed the sidebar_whitelist branch 2 times, most recently from a485073 to 0b953ed Compare April 8, 2016 00:15
@@ -23,6 +23,72 @@ limitations under the License.
</tr>
<tr>
<td width="40%"><strong>Availability</strong></td>
<td><div>Experimental</div><div>Work in progress.</div></td>
<td><div><a href="https://www.ampproject.org/docs/reference/experimental.html">Experimental</a>; no validations yet.</div><div>Work in progress.</div></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like at this point we can already file the validator request, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, please do file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the changes are already on goldmine :) ill have to file bugs for some minor changes.

@dvoytenko
Copy link
Contributor

One significant change requested to improve scan. Otherwise, LGTM.

@camelburrito camelburrito force-pushed the sidebar_whitelist branch 3 times, most recently from 8b4ff2e to d4608fe Compare April 8, 2016 20:54
@camelburrito camelburrito merged commit c08566f into ampproject:master Apr 8, 2016
@camelburrito camelburrito deleted the sidebar_whitelist branch April 8, 2016 21:41
@camelburrito camelburrito mentioned this pull request Apr 13, 2016
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.

2 participants