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

📦 NEW: Compile multiple CSS files into different directories #125

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mintbird
Copy link

Description

Refer to #103 Add capability to compile multiple CSS files into different directories
With this feature, user can add scss files and specify the location of compiled css file for each scss file.
The steps of compilation and configuration is the same as task "styles" and "stylesRTL".
The new task "addonStyles" will be processed with "config.watchStyles"

Screenshots:

No.

Types of changes

NEW: Compile multiple CSS files into different directories

How Has This Been Tested?

Tested with the default configuration
Empty configuration "addonStyles" will not bug the task.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has extensive inline documentation like the rest of WPGulp.

Copy link
Owner

@ahmadawais ahmadawais left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome PR. Take a look at the comments.

src/wpgulp.config.js Show resolved Hide resolved
var tasks = config.addonStyles.map( function ( addon ) {

return gulp
.src( addon.styleSRC, { allowEmpty: true })
Copy link
Owner

Choose a reason for hiding this comment

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

I wish there was less repetition. This styles task is repeated four times now, maybe we can make a function that can be used everywhere in styles, stylesRTL and addonStyles/RTL tasks? Would you be up for it?

1. New function processStyle() for process any selected SCSS files with RTL option.

2. Remove example of addonStyles from configuration file and add description in README.md
@mintbird
Copy link
Author

Fixed from your comment. All pipes related to processing SCSS file was moved to the new function processStyle and the example configuration was removed and described in README.md instead.

README.md Outdated
@@ -139,6 +139,22 @@ Configure the project paths and other variables inside the `wpgulp.config.js` fi

![wpgulp config](https://on.ahmda.ws/f2ca9bb4a740/c)

#### Add-on Styles Option
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the right place for content like this. Kindly, create a new heading after installation call it, FAQs, ask a question and then answer it. 👍

src/gulpfile.babel.js Show resolved Hide resolved
*/
gulp.task( 'styles', () => {
return processStyle(
gulp.src( config.styleSRC, { allowEmpty: true })
Copy link
Owner

Choose a reason for hiding this comment

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

Missing styleDestination.

src/gulpfile.babel.js Show resolved Hide resolved
function processStyle( gulpStream, processOptions = {} ) {
processOptions = defaults( processOptions, {
isRTL: false,
styleDestination: './',
Copy link
Owner

Choose a reason for hiding this comment

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

Even with simple styles the styleDestination should always be provided for folks who change the location of it for some reason.

@ahmadawais
Copy link
Owner

To write commits for this repo, it's important that you follow the Emoji-log commit standard. Otherwise, I'll have to squash everything into one single commit. And you'll get only one commit contribution props.

Also check the changes I asked for. This PR is looking pretty good.

@mintbird
Copy link
Author

Fixed.

You can squash the commits.

@isvictorious
Copy link

When can this be merged?

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.

5 participants