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

Make Gruntfile more foolproof #518

Merged
merged 6 commits into from
Dec 22, 2017
Merged

Make Gruntfile more foolproof #518

merged 6 commits into from
Dec 22, 2017

Conversation

jessetan
Copy link
Contributor

This should reduce some issues where incorrect files are committed or released:

  • Clean all output before building. Prevents outdated/orphaned files from being committed
  • Run copy:fonts when building. Prevents issues with outdated or missing font files (no need to repeat Run grunt fonts #506 manually)

@Blendify
Copy link
Member

Blendify commented Dec 20, 2017 via email

@jessetan
Copy link
Contributor Author

jessetan commented Dec 20, 2017

Because Modernizr is only a committed file (probably downloaded from the website), it is not built by any grunt task.
It would be good to build one using https://github.com/Modernizr/grunt-modernizr.

@Blendify
Copy link
Member

Blendify commented Dec 20, 2017 via email

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

LGTM, along with @Blendify's comment, which might be useful.

@@ -160,6 +162,6 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('grunt-browserify');

grunt.registerTask('fonts', ['clean:fonts','copy:fonts']);
Copy link
Member

Choose a reason for hiding this comment

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

I think grunt fonts should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one, done

Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

  • Comment about minimizer
  • Remove the fonts task register

@jessetan
Copy link
Contributor Author

Is Modernizr actually used for anything? grunt-modernizr does not find anything apart from an input[search] element, but there is no JS or CSS that uses the functionality provided by Modernizr.

The only reasons I can think for keeping this is:

  • The included version of Modernizr includes an html5 shiv for older browsers
  • Pages that use the theme may rely on the presence of Modernizr, although they could include one themselves easily.

Perhaps we can consider removing Modernizr? I suggest we leave this PR as-is and do any changes related to Modernizr in a separate PR.

Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

I am ok with that

@ericholscher ericholscher merged commit b2c8c0c into master Dec 22, 2017
@ericholscher
Copy link
Member

👍

This was referenced Dec 22, 2017
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.

3 participants