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

Source maps #646

Merged
merged 14 commits into from
Apr 14, 2015
Merged

Source maps #646

merged 14 commits into from
Apr 14, 2015

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Apr 8, 2015

Changes:

  • creates a LARGO_DEBUG constant that controls whether or not minified assets are used in:
    • inc/enqueue.php:
      • largo_enqueue_js
      • largo_enqueue_admin_js
    • inc/featured-media.php
      • largo_enqueue_featured_media_js
    • inc/post-metaboxes.php
      • largo_custom_sidebar_js
    • inc/term-icons.php
      • Largo_Term_Icons->admin_enqueue_scripts()
    • inc/update/php
      • largo_update_page_enqueue_js
  • adds CSS sourcemaps to the unminified CSS files created by grunt watch
  • adds CSS minification to the grunt watch task
  • adds js/widgets-php.js to the grunt uglify task

Why?

Minified assets: Faster to download for the user.

Sourcemaps: When you go to the web inspector to see why the CSS is messed up, instead of saying style.css on line 3040, it tells you that the rule is in header.less on line 34.

Inline sourcemaps: This is a workaround. grunt-contrib-less doesn't do multiple sourcemap file output when you compile multiple LESS files concurrently. More about that here. This PR puts the sourcemaps in the unminified CSS files, linking to the LESS files in the theme.

Inline sourcemaps not in minified CSS: The sourcemap for Largo's main style.css is near 250,000 characters. Putting that in the minified, faster-to-download asset doesn't seem right.

Changes to Gruntfile:
- sourcemaps require the URI path, not the filesystem path, so var path is created to hold the path of the file relative to the Wordpress root.
- cssmin is added to grunt watch to make sure that the minified assets are updated

Questions:

  • Should the less:production task be removed from the grunt less task, since the production css assets are created by cssmin?
  • How far should we go in minifying CSS and JavaScript assets? A short list of unminified assets:
    • js/jquery.idTabs.js: only included on single pages, should probably be enqueued from the widget that uses it instead of on all single pages. Adding this to the uglify task results in an output file named jquery.min.js instead of jquery.idTabs.min.js because grunt reads extensions from the first period. That is a wontfix behavior. If we were to minify this, we should consider renaming it to jquery-idTabs.js
    • homepages/assets/js/single.js: used on the homepage
    • js/tinymce/pluins/largo/editor_plugin.js and tinymce.js: only used in the admin
    • js/select2: the locale files
    • inc/wp-taxonomy-landing/series.js and series.css: only used in the admin, enqueued in inc/wp-taxonomy-landing/functions/cftl-admin.php

For #432 and #369

@benlk benlk added this to the 0.5 milestone Apr 8, 2015
Conflicts:
	functions.php

Not sure how well this merge went, the code looks correct though.
@benlk
Copy link
Collaborator Author

benlk commented Apr 9, 2015

Change 2015-04-09: added "Why" section to PR description.

@rnagle
Copy link

rnagle commented Apr 14, 2015

This is awesome. I consider this a huge step forward for Largo. Thanks for the effort.

Couple things:

  • The "Why?" section in the pull request should probably end up in the docs somewhere. We should also add some general docs section about LARGO_DEBUG -- how and when to use it.
  • Regarding your questions:
    • Yes, I think it makes sense to remove the less:production task
    • This is an awesome start and lays the the groundwork for the part of the site where asset minification matters most -- the front-end of the theme. Tackling other assets would be "nice to have" and can be backlogged for now.

Going to merge this and create new issues based on feedback above.

rnagle added a commit that referenced this pull request Apr 14, 2015
Source maps, LARGO_DEBUG switch for determining when to use minified assets.
@rnagle rnagle merged commit 96d26b7 into develop Apr 14, 2015
@benlk benlk deleted the source-maps branch May 4, 2015 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants