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

Grunt: refactor #566

Closed
wants to merge 9 commits into from
Closed

Grunt: refactor #566

wants to merge 9 commits into from

Conversation

Blendify
Copy link
Member

@Blendify Blendify commented Jan 31, 2018

  • Cleanup font copying
  • Create new tasks: install, docs

@Blendify Blendify changed the title Dont run sphinx on build task Grunt: refactor Jan 31, 2018
@davidfischer davidfischer added the PR: work in progress Pull request is not ready for full review label Feb 1, 2018
@davidfischer
Copy link
Contributor

The Gruntfile.js currently has parse errors so I'm marking this "work in progress" for now.

- to many braces
- Split up long path lines
@Blendify
Copy link
Member Author

Blendify commented Feb 1, 2018

Ah should work, I was rushed when I wrote this one.

@@ -181,5 +169,7 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('grunt-browserify');

grunt.registerTask('default', ['exec:bower_update','clean','copy:fonts','sass:dev','browserify:dev','exec:build_sphinx','connect','open','watch']);
grunt.registerTask('build', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify','exec:build_sphinx']);
grunt.registerTask('build', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This removed the actual building of the docs. Was that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the way I see this is that grunt build should just build the theme related files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with it. I just want to be sure.

Gruntfile.js Outdated
'bower_components/robotoslab-googlefont/RobotoSlab-Bold.ttf',
'bower_components/robotoslab-googlefont/RobotoSlab-Regular.ttf',
'bower_components/inconsolata-googlefont/Inconsolata-Bold.ttf',
'bower_components/inconsolata-googlefont/Inconsolata-Regular.ttf']
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a syntax error here. A trailing comma is required.

@davidfischer
Copy link
Contributor

🚢

@davidfischer davidfischer removed the PR: work in progress Pull request is not ready for full review label Feb 1, 2018
@Blendify
Copy link
Member Author

Blendify commented Feb 3, 2018

@ericholscher do you agree with the changes done to the grunt tasks?

- use build tasks for install
- rename clean:build to clean:docs
Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

LGTM

I don't think we have much reason to split up the font copy operations, so 👍

Copy link
Contributor

@jessetan jessetan left a comment

Choose a reason for hiding this comment

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

Refactoring looks good, some remarks regarding new/changed Grunt tasks.

Gruntfile.js Outdated
@@ -181,5 +169,7 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('grunt-browserify');

grunt.registerTask('default', ['exec:bower_update','clean','copy:fonts','sass:dev','browserify:dev','exec:build_sphinx','connect','open','watch']);
grunt.registerTask('build', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify','exec:build_sphinx']);
grunt.registerTask('build', ['clean','copy:fonts','sass:build','browserify:build','uglify']);
grunt.registerTask('install', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify']);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for the new install step? I think we should have a single command that produces a built theme with CSS and JS, completely from scratch. It looks like you're doing that with install, but what would we need build for then?

Gruntfile.js Outdated
grunt.registerTask('build', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify','exec:build_sphinx']);
grunt.registerTask('build', ['clean','copy:fonts','sass:build','browserify:build','uglify']);
grunt.registerTask('install', ['exec:bower_update','clean','copy:fonts','sass:build','browserify:build','uglify']);
grunt.registerTask('docs', ['exec:build_sphinx','connect','open']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also call all the stuff that copies and builds assets (copy, sass, browserify). The docs need these assets for proper display and they might not be there if the were cleaned.

@Blendify
Copy link
Member Author

I decided to revert the changes that we were not sure about. Im not exactly sure what I was going for but I like how it is now.

@jessetan
Copy link
Contributor

Do you want to keep the docs task as it is now? It will not have styling if run after a clean since it does not build the assets.

@agjohnson agjohnson added this to the v0.4.0 milestone Mar 29, 2018
@agjohnson agjohnson modified the milestones: v0.4.0, 0.5 Jun 7, 2018
@Blendify Blendify closed this Dec 5, 2018
@Blendify Blendify deleted the grunt-build-nodocs branch December 5, 2018 01:18
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.

4 participants