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.

Sign up for GitHub

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

Jump to bottom

Apply dynamic css classes on <body> #2761

Closed
rudygalfi opened this issue Mar 30, 2016 · 8 comments
Closed

Apply dynamic css classes on <body> #2761

rudygalfi opened this issue Mar 30, 2016 · 8 comments
Assignees
@dvoytenko
Labels
Related to: Documentation
Milestone
Sprint: 2016-04-14

Comments

@rudygalfi
Copy link
Contributor

https://www.ampproject.org/docs/reference/extended/amp-dynamic-css-classes.html says they are added to the "HTML element", implying <html>. There was some discussion that it's actually <body>, so this needs to be confirmed one way or the other.

cc @ericlindley-g
@rudygalfi rudygalfi added this to the Sprint: 2016-04-14 [next] milestone Mar 30, 2016
@jridgewell
Copy link
Contributor

It's <html>.

@rudygalfi
Copy link
Contributor Author

Thanks. No doc change needed.

@dvoytenko dvoytenko reopened this Mar 30, 2016
@dvoytenko
Copy link
Contributor

We need to discuss: all other classes are applied on body. Ideally we'd put them all in one place, but at this point it'd be hard to move other classes to html.

@rudygalfi
Copy link
Contributor Author

What are the options?

We could:

  • add dynamic CSS classes also to <body> while also keeping them on <html>
  • add, but also remove from <html> (how would this work?)

@dvoytenko
Copy link
Contributor

The only option we can consider at this time (as opposed to keep it as is) is to move classes to body for all cases.

@jridgewell
Copy link
Contributor

@dvoytenko: Dynamic CSS Classes hasn't been officially released yet (it's still behind an experiment in the latest release). Should we change this before pushing out the next prod?

@rudygalfi
Copy link
Contributor Author

For some reason I was thinking it was stable. Yes, we should definitely make that change.

@rudygalfi rudygalfi changed the title Confirm element where dynamic css classes are applied Apply dynamic css classes on <body> Apr 1, 2016
@jridgewell
Copy link
Contributor

It's stable now, but we haven't pushed a new release. 😉

@jridgewell jridgewell mentioned this issue Apr 1, 2016
Merged
jridgewell added a commit to jridgewell/amphtml that referenced this issue Apr 4, 2016
@jridgewell
Add Dynamic CSS Classes to Body
f5cf66d 
Also adds the required script tag documentation. There are no validation
errors.

Fixes ampproject#2761.
Fixes ampproject#2747.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dvoytenko dvoytenko

Labels
Related to: Documentation
Projects
None yet
Milestone
Sprint: 2016-04-14
Development

No branches or pull requests
3 participants
@jridgewell @dvoytenko @rudygalfi