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

Fixes 3872: Provides a better customizable application layout that #1088

Merged
merged 1 commit into from Dec 19, 2013

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Dec 13, 2013

conforms to HTML5 best practices.

This change aims to provide a more customizable application layout by first
splitting the base layout, menu and content out into their own view files.
The base layout provides generic hooks and only the most basic common data
such as application javascript, stylesheets, inclusion of the menu and auth
token. Javascript inclusion is moved to the bottom of the page for page
loading efficiency. Further, all inline scripts are placed into the same
tag for browser loading efficiency. Stylesheet and generic head hooks are
split out for better readability. Lastly, this includes some updates to the
overall layout by including current HTML5 best practices for detecting and
declaring the doctype for polyfills and proper meta tags.

@ohadlevy
Copy link
Member

@abenari could you please review? thanks!

<!--[if lt IE 7]> <html lang="<%= I18n.locale %>" class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]-->
<!--[if IE 7]> <html lang="<%= I18n.locale %>" class="no-js lt-ie9 lt-ie8"> <![endif]-->
<!--[if IE 8]> <html lang="<%= I18n.locale %>" class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html lang="<%= I18n.locale %>" class="no-js"> <!--<![endif]-->
Copy link
Member

Choose a reason for hiding this comment

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

whats no-js? do we use it anywhere? also, are we using the lt-* classes anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

These declarations serve to provide a consistent set of classes to be able to apply CSS overrides for older IE browsers. While we are not using them specifically as the moment, we may take advantage as we do more extensive browser testing. Similarly for no-js class - a standard way to apply styling for anything no-js related. This comes from https://github.com/h5bp/html5-boilerplate/blob/v4.3.0/doc/html.md#conditional-html-classes but is not strictly necessary to have.

@ohadlevy
Copy link
Member

tested it with latency, and it feels much better!
@abenari could you do double check ?

@ehelms
Copy link
Member Author

ehelms commented Dec 17, 2013

An expedited review would be nice as I am blocked by this. Thanks.

@abenari
Copy link
Member

abenari commented Dec 18, 2013

loading js at the bottom of the page makes too many pages look bad, that needs to be reverted. Other than that the patch is ok.

conforms to HTML5 best practices.

This change aims to provide a more customizable application layout by first
splitting the base layout, menu and content out into their own view files.
The base layout provides generic hooks and only the most basic common data
such as application javascript, stylesheets, inclusion of the menu and auth
token. Javascript inclusion is moved to the bottom of the page for page
loading efficiency. Further, all inline scripts are placed into the same
tag for browser loading efficiency. Stylesheet and generic head hooks are
split out for better readability. Lastly, this includes some updates to the
overall layout by including current HTML5 best practices for detecting and
declaring the doctype for polyfills and proper meta tags.
@ehelms
Copy link
Member Author

ehelms commented Dec 18, 2013

Updated to move JavaScript into the header entirely.

@abenari abenari merged commit 19f5592 into theforeman:develop Dec 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants