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

Add gruntfile or task runner of choice for bp-nouveau #121

Closed
mercime opened this Issue Apr 5, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@mercime
Member

mercime commented Apr 5, 2017

Specially for processing and linting Sass files at this point. It makes it easier for contributors to submit pull requests.

In current setup, one will need to copy over the Sass files to a separate folder from nouveau and use grunt CLI, gulp CLI, Ruby sass CLI, or npm CLI to do this. And then copy over the revised Sass file and generated CSS files to bp-nouveau. Alternatively, one can add own processor and linter in bp-nouveau and just add e.g. gruntfile, gulpfile, or ruby sass-generated elements into own gitignore

I don't know when bp-nouveau will be merged into BP core, but I will presume that the merge will be done when all issues and testing have been addressed in this repo. I expect merge sometime right after WordCamp Europe before string freeze.

@mercime

This comment has been minimized.

Show comment
Hide comment
@mercime

mercime Apr 7, 2017

Member

Per dev chat last April 5, task runner will be added sometime, hopefully soon - https://wordpress.slack.com/archives/C02RQBYUG/p1491419356556648

Member

mercime commented Apr 7, 2017

Per dev chat last April 5, task runner will be added sometime, hopefully soon - https://wordpress.slack.com/archives/C02RQBYUG/p1491419356556648

hnla added a commit that referenced this issue Apr 8, 2017

Add package.json, gruntfile, stylelintrc, jshintrc, jshintignore
Commit adds a basic package to run:
* Watch task for scss/css compilation on the fly.
* Manual rtlcss build.
* Manual scss/css linting.

See #121
@hnla

This comment has been minimized.

Show comment
Hide comment
@hnla

hnla Apr 8, 2017

Member

Commit adds my working files for grunt tasks.

To note:
This is primarily to run:

$ grunt watch

which will compile the scss changes to css from the scss directory so any changes to the partial import files will trigger compilation of the main buddypres.scss and in turn generate the css file.

$ grunt stylelint

Manually runs linting

$ grunt rtlcss

Will run a rtl version of the css file, as will $ grunt commit to run stylelint, rtlcss.

The file is scrappy but serves the simple workflow required at this stage working in the plugin version.

I envisage , when merged to core, that this set of files is redundent, we just need to modify the main BP gruntfile to include the watch task and new paths to files.

Member

hnla commented Apr 8, 2017

Commit adds my working files for grunt tasks.

To note:
This is primarily to run:

$ grunt watch

which will compile the scss changes to css from the scss directory so any changes to the partial import files will trigger compilation of the main buddypres.scss and in turn generate the css file.

$ grunt stylelint

Manually runs linting

$ grunt rtlcss

Will run a rtl version of the css file, as will $ grunt commit to run stylelint, rtlcss.

The file is scrappy but serves the simple workflow required at this stage working in the plugin version.

I envisage , when merged to core, that this set of files is redundent, we just need to modify the main BP gruntfile to include the watch task and new paths to files.

@mercime

This comment has been minimized.

Show comment
Hide comment
@mercime

mercime Apr 11, 2017

Member

The file is scrappy but serves the simple workflow required at this stage working in the plugin version. I envisage , when merged to core, that this set of files is redundent, we just need to modify the main BP gruntfile to include the watch task and new paths to files.

Agreed. Thanks hnla. Will be testing this in a little bit.

Member

mercime commented Apr 11, 2017

The file is scrappy but serves the simple workflow required at this stage working in the plugin version. I envisage , when merged to core, that this set of files is redundent, we just need to modify the main BP gruntfile to include the watch task and new paths to files.

Agreed. Thanks hnla. Will be testing this in a little bit.

@mercime

This comment has been minimized.

Show comment
Hide comment
@mercime

mercime Apr 11, 2017

Member

Tested this a few minutes ago and got
Warning: Task "uglify" not found. Use --force to continue.

Checked out Gruntfile.js and there's no uglify configured there while we're registering uglify as default task
grunt.registerTask('default', ['uglify', 'postcss']);
See https://github.com/gruntjs/grunt-contrib-uglify#usage-examples

I understand that this was primarily uploaded to take care of the Sass files so we can help out. Thank you. Just checking if the uglify configuration was mistakenly removed when you cleaned up your Gruntfile.js before committing. If not, then disregard this message and I'll move on to other tickets 👍

Member

mercime commented Apr 11, 2017

Tested this a few minutes ago and got
Warning: Task "uglify" not found. Use --force to continue.

Checked out Gruntfile.js and there's no uglify configured there while we're registering uglify as default task
grunt.registerTask('default', ['uglify', 'postcss']);
See https://github.com/gruntjs/grunt-contrib-uglify#usage-examples

I understand that this was primarily uploaded to take care of the Sass files so we can help out. Thank you. Just checking if the uglify configuration was mistakenly removed when you cleaned up your Gruntfile.js before committing. If not, then disregard this message and I'll move on to other tickets 👍

@mercime

This comment has been minimized.

Show comment
Hide comment
@mercime

mercime Apr 11, 2017

Member

We should add "Do not run grunt in command line after npm install so that the package.json will not be updated for this project? See what happened in this commit.

Will revert the package.json to what you uploaded earlier.

Member

mercime commented Apr 11, 2017

We should add "Do not run grunt in command line after npm install so that the package.json will not be updated for this project? See what happened in this commit.

Will revert the package.json to what you uploaded earlier.

mercime added a commit that referenced this issue Apr 11, 2017

@hnla

This comment has been minimized.

Show comment
Hide comment
@hnla

hnla Apr 11, 2017

Member

Not sure what happened there, so running '$ grunt` after install updates package.json? There's a lot I don't comprehend about grunt.

As for the uglify task, it's odd as I don't get errors but the registerTask is a legacy so probably best to simply comment out or delete, were installing the module but as you point out there's no task setup and no particular need for it in this grunt file.

Member

hnla commented Apr 11, 2017

Not sure what happened there, so running '$ grunt` after install updates package.json? There's a lot I don't comprehend about grunt.

As for the uglify task, it's odd as I don't get errors but the registerTask is a legacy so probably best to simply comment out or delete, were installing the module but as you point out there's no task setup and no particular need for it in this grunt file.

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Apr 23, 2017

Member

Can this issue be assigned to me please 😄

Member

ntwb commented Apr 23, 2017

Can this issue be assigned to me please 😄

@hnla

This comment has been minimized.

Show comment
Hide comment
@hnla

hnla Apr 23, 2017

Member

Asked lead devs to sort out the access as you're not shown in the assignees listing for some reason.

Where making any changes to the gruntfile can we ensure that the workflow is maintained, it's admittedly messy & could do with tidying up but nervous about breaking workflow as trying to commit quite a bit at the moment with what time available.

Member

hnla commented Apr 23, 2017

Asked lead devs to sort out the access as you're not shown in the assignees listing for some reason.

Where making any changes to the gruntfile can we ensure that the workflow is maintained, it's admittedly messy & could do with tidying up but nervous about breaking workflow as trying to commit quite a bit at the moment with what time available.

@ntwb ntwb referenced this issue Apr 27, 2017

Merged

Grunt update #136

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Apr 27, 2017

Member

PR #136: Grunt: Major refactor of Gruntfile.js
• There are 3 primary Grunt tasks now:
grunt lint runs the JSHint and stylelint grunt tasks
grunt build builds the CSS & RTL CSS from SCSS
grunt runs both grunt lint and grunt build

Member

ntwb commented Apr 27, 2017

PR #136: Grunt: Major refactor of Gruntfile.js
• There are 3 primary Grunt tasks now:
grunt lint runs the JSHint and stylelint grunt tasks
grunt build builds the CSS & RTL CSS from SCSS
grunt runs both grunt lint and grunt build

@hnla

This comment has been minimized.

Show comment
Hide comment
@hnla

hnla Apr 27, 2017

Member

@netweb
Any reason we shouldn't just merge this straight into 'master'?

Member

hnla commented Apr 27, 2017

@netweb
Any reason we shouldn't just merge this straight into 'master'?

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Apr 28, 2017

Member

@hnla, it was late last night and I wanted to double check things this morning with fresh eyes 👀

Member

ntwb commented Apr 28, 2017

@hnla, it was late last night and I wanted to double check things this morning with fresh eyes 👀

@ntwb ntwb closed this in #136 Apr 28, 2017

@hnla

This comment has been minimized.

Show comment
Hide comment
@hnla

hnla Apr 28, 2017

Member

@netweb understood, thanks for sorting.

Member

hnla commented Apr 28, 2017

@netweb understood, thanks for sorting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment