Code Review: CSS/JS Abstraction and Theme improvements #958

rjmackay · 2012-12-03T03:57:02Z

This a fairly major set of changes and could do with a serious code review.
Here's the main things this does:

Add theme inheritance and css/js overriding
- This still default to including the default theme
- Allows themes to specify CSS/JS files to include through readme.txt
- Allow themes to override CSS/JS from parent theme by include a file of the same name
Split out themes/default/css/style.css
Handle all CSS / JS includes through 1 library: Requirements
- This enables us to combine and compress these files
- Requirements library is ported from another framework, but has had serious modification so needs some review too (however I haven't rewritten it all so haven't enforced coding standards everywhere)
- We're adding CSSMin and JSMin to compress files
- A bunch of new options in application/config/requirements.php
Add support for RTL css files through Requirements library.
- All CSS files can be replace by a file of the same name with the -rtl suffix.

There are also other fixes that were enabled by the refactoring, and some general JS/CSS cleanup.

Issues addressed: #338 #550 #301 #261

I've started documenting all this here: https://wiki.ushahidi.com/display/WIKI/Managing+CSS+and+JS+in+Ushahidi

Any feedback appreciated, even if its to tell me I'm crazy and I've over thought the whole thing.

ekala · 2012-12-04T14:32:02Z

application/controllers/main.php

@@ -135,6 +135,8 @@ public function __construct()
 			$this->template->header->header_nav->loggedin_user = Auth::instance()->get_user();
 		}
 		$this->template->header->header_nav->site_name = Kohana::config('settings.site_name');
+
+		Event::add('ushahidi_filter.view_pre_render-layout', array($this, '_pre_render'));


Docs for this hook?

As for the name, we've implicitly settled on using _ as a separator. So ushahidi_filter.view_pre_render_layout

I'll add a doc on this line.
This hook actually runs here: https://github.com/ushahidi/Ushahidi_Web/blob/develop/application/libraries/MY_View.php#L42
The last part -layout .. is based on the view name..
I've got - as a separator because I wanted something the split view_pre_render from the view name and have slightly less chance of naming collisions.

I can change that in MY_View.php if needed though.

I see. In that case, perhaps a . separator? This way, the last part of a view_pre_render hook will always be the view name.

…di#550 * Fix other themes for renaming rapidxwpr to wrapper ushahidi#550

* pngFix can be run everywhere anyway. * runScheduler was never being called

* Convert Themes library to use requirements * Convert register_themes.php to use requirements * Convert plugin helper to use requirements * Move theme js to use Requirements library * Stop passing script tag in api_url config settings. Set url in settings and handle the actual tag through Requirements * Fix path for file to stop it suffix'ing remote urls * Render Requirements through Themes library for frontend ushahidi#338 This makes it easier to update old themes as they can still use header_block() * Make admin/members layout call Requirement::render() since they don't have header_block()

* Only include jqplot js+css if needed * Remove inline css and use class instead * Remove graph div when disabled

* Rename style.css in other themes to avoid overriding default style.css ushahidi#301 * Move the Requirements calls from register_themes.php into Themes library

* Split style.css into base.css and style.css * Tweak alerts radius map not to use .map class Use map-wrapper class instead as this avoids conflicts with div.map on main page. This Simplifies base.css so it doesn't need to style the map-wrapper too * Move accordion styling to its own file * Add basic style for reports submit and get alerts in base.css * Remove some old unused CSS too

…shahidi#550 * Allow themes to specify addition css/js files to include in readme.txt * Only include _default.css, style.css or THEMENAME.css by default * Update existing themes to specify JS files

… css ushahidi#301 * Can now override openlayers.css and jquery-ui-themeroller.css * themedCSS() will search for CSS in themes, then module (ie. plugin) then media/css

* Move iehacks CSS into default theme * Added Requirements::ieCSS() and Requirements::ieThemedCSS()

* Also removing some unused images * Remove duplicate images * Remove duplicate jquery.timeago JS in ci_cumulus theme

…emes ushahidi#338 * Also updates all admin controller to set XYZ_enabled in themes * Add requirements at the last minute through views_pre_render event * Pass api_url_all as an array of urls, for easier use with Requirements * Make Requirements handle arrays of css or js files * Move some admin js to media/js/admin.js instead of inline

…hahidi#338 * Start calling requirements from pre-render event for main controller too * Backend and frontend requirements are enabled through a variable on Themes. * Slider, timeline and hovertip are now enabled like other js libraries rather than always included or include based on $main_page * Removes unused flot js and excanvas js * Openlayers and jquery-ui-themeroller are no longer themedcss but included early so can be overridden via theme css files anyway. * Move admin header/footer block render to pre render event

* Replace jquery.ui.min.js with version including accordion Previously only included features used in the admin. * No longer using the google CDN for this as per ushahidi#365 * Previously the admin used its own jquery UI, now both admin and frontend use the same version

Conflicts: application/views/admin/layout.php

* Pass themes object to themes_add_requirements action * Add event for adding requirements before admin/frontend stuff

* Give all CSS/JS uniquenessIDs, based on filename if not supplied * Require uniquenessID on custom CSS/JS/HeadTag * Handle handle write_js_to_body option in Themes library not Requirements * Give themedCSS a better uniqueid * Split out Render by type instead of location * Remove combined files code - rewrite later * Remove auto js/css extension adding magic * Add notice for missing JS/CSS files

* Add option to requirements to support combining files * Add groups for base and admin combined files * Move admin/all.css to admin.css so url() links work when combined * Add CDN support for combined files * Auto upload combined files to the CDN * Fix minor issues in Cloudfiles classes - probably related to PHP 5.4 * Make cdn helper handle files not located in media/uploads or already with full path.

* RTL languages are detected based on special core.text_direction i18n string * If language is RTL we check for CSS files with the same name but a -rtl suffix * Support -rtl versions of combined files too.

* Auto combine CSS files for themes * Including the Minify_CSS_UriRewriter to fix url() links in theme css * Include CSSMin to minify CSS files * Add extra static call CSSMin::go()

@ekala

Now using . as separator instead of - As per feedback from @ekala this is a better match for the existing naming convention.

* fix block() to handle IDs and full filenames * fix path_for_file() to handle query strings w/o mtime * fix delete_combined_files()

rjmackay · 2012-12-10T22:57:14Z

Rebasing from develop and fixing conflicts.

…g-338 Code Review: CSS/JS Abstraction and Theme improvements

kamaulynder · 2013-02-15T14:39:18Z

Applied the patch and after a few hitches with, finally they were cleanly applied. Looking good, functional tests will be done as we get close to tagging 2.7 for release.
@eyedol and I tested this and agreed we merge it in and handle anything after functional tests are done. Awesome work @rjmackay

rjmackay · 2013-02-19T00:14:31Z

Ok great thanks for the effort!
on key this to test pre 2.7: what will this break in custom themes and can we avoid those breaks. I've had to shelve that for uchaguzi.

ekala reviewed Dec 4, 2012
View reviewed changes

rjmackay mentioned this pull request Dec 9, 2012

Page Load Analysis Issues #839

Closed

rjmackay added 23 commits December 11, 2012 11:52

Rename rapidxwpr div class to wrapper - since thats what it is ushahi…

f2e4f71

…di#550 * Fix other themes for renaming rapidxwpr to wrapper ushahidi#550

Move pngFix into global.js and remove runScheduler function ushahidi#550

27d0645

* pngFix can be run everywhere anyway. * runScheduler was never being called

Porting Requirements library ushahidi#338

3cfa8d9

Clean up timeline on/off css/js ushahidi#598

4a3563f

* Only include jqplot js+css if needed * Remove inline css and use class instead * Remove graph div when disabled

Add theme inheritance ushahidi#338

3f248f7

* Rename style.css in other themes to avoid overriding default style.css ushahidi#301 * Move the Requirements calls from register_themes.php into Themes library

Switch themes to only include a limited set of CSS files by default u…

be48891

…shahidi#550 * Allow themes to specify addition css/js files to include in readme.txt * Only include _default.css, style.css or THEMENAME.css by default * Update existing themes to specify JS files

Make Requirements::themedCSS() work and allow themes to override more…

3b0eb4d

… css ushahidi#301 * Can now override openlayers.css and jquery-ui-themeroller.css * themedCSS() will search for CSS in themes, then module (ie. plugin) then media/css

Add support for IE stylesheets to Requirements ushahidi#338

4f81622

* Move iehacks CSS into default theme * Added Requirements::ieCSS() and Requirements::ieThemedCSS()

Share images between frontend and admin where possible ushahidi#550

0c1d810

* Also removing some unused images * Remove duplicate images * Remove duplicate jquery.timeago JS in ci_cumulus theme

Convert members controllers to use requirements for js/css

acc0095

Don't render header/footer blocks till the last minute

7e89c9a

Move Requirements config into a config file ushahidi#338

e0f95f9

Move admin ie CSS in to media/css/ie6.css ushahidi#550

1cc5bfe

Conflicts: application/views/admin/layout.php

More/Better theme requirements Events

0eff814

* Pass themes object to themes_add_requirements action * Add event for adding requirements before admin/frontend stuff

Move hook/register_themes code into Themes library

346fa82

Combine header_block() and admin_header_block()

cfb5dcf

rjmackay added 7 commits December 11, 2012 11:55

Add support for -rtl.css files ushahidi#261

32f5e11

* RTL languages are detected based on special core.text_direction i18n string * If language is RTL we check for CSS files with the same name but a -rtl suffix * Support -rtl versions of combined files too.

Combine the theme files

c2f73b6

* Auto combine CSS files for themes * Including the Minify_CSS_UriRewriter to fix url() links in theme css * Include CSSMin to minify CSS files * Add extra static call CSSMin::go()

Change view_pre_render event naming convention ushahidi#958

931c528

Now using . as separator instead of - As per feedback from @ekala this is a better match for the existing naming convention.

Minor: Fix code formatting in Themes library ushahidi#958

90c664f

Remove dupe copyright note in Requirements library ushahidi#958

c0c6841

Add unit tests for Requirements library

6ff0098

Update requirements library to fix issues found by unit testing

a3e6b4d

* fix block() to handle IDs and full filenames * fix path_for_file() to handle query strings w/o mtime * fix delete_combined_files()

kamaulynder added a commit that referenced this pull request Feb 15, 2013

Merge pull request #958 from rjmackay/feature/abstract-css-js-handlin…

697d6ce

…g-338 Code Review: CSS/JS Abstraction and Theme improvements

kamaulynder merged commit 697d6ce into ushahidi:develop Feb 15, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review: CSS/JS Abstraction and Theme improvements #958

Code Review: CSS/JS Abstraction and Theme improvements #958

rjmackay commented Dec 3, 2012

ekala Dec 4, 2012

rjmackay Dec 4, 2012

ekala Dec 4, 2012

rjmackay commented Dec 10, 2012

kamaulynder commented Feb 15, 2013

rjmackay commented Feb 19, 2013

Code Review: CSS/JS Abstraction and Theme improvements #958

Code Review: CSS/JS Abstraction and Theme improvements #958

Conversation

rjmackay commented Dec 3, 2012

ekala Dec 4, 2012

Choose a reason for hiding this comment

rjmackay Dec 4, 2012

Choose a reason for hiding this comment

ekala Dec 4, 2012

Choose a reason for hiding this comment

rjmackay commented Dec 10, 2012

kamaulynder commented Feb 15, 2013

rjmackay commented Feb 19, 2013