liferay / liferay-theme-tasks

A set of tasks for building and deploying Liferay Portal themes.
18 stars 24 forks source link

SASS includePaths #35

Closed justLuiz closed 7 years ago

justLuiz commented 7 years ago

if a custom includePath has added. Example:

sassOptions: {
  includePaths: ['../some/path', 'another/path']
}

the default includePath are overwritten. at build.js, line 89, lodash assign method dont merge arrays.

natecavanaugh commented 7 years ago

Hi Luis, Thanks for reporting this issue, but I'm not sure what would be the ideal way for you to be able to displace the includePaths if desired, or merging it with the existing array.

I'm open to ideas about which behavior should be default, as well as to how you would accomplish the opposite behavior.

One issue is that, even if we used _.merge, which will merge the two arrays, it will overwrite the items in the destination array with the same index, so that the final result of:

    _.merge({
      includePaths: ['default1', 'default2']
     },
      {
         includePaths: ['new1', 'new2', 'new3']
    });

would look something like this:

{
   includePaths: ['new1', 'new2', 'new3']
}

Which still wouldn't be what you want.

One idea I can think of is that we could allow passing the includePaths as a function, and if so, pass in the current includePaths so you can decide what to return (whether a whole new array, or the same one with different options).

I'm imagining though, that this might be better if instead of selectively making different options a function, if it would instead be better to allow the entire sassOptions property to be a function which accepts the defaults, and returns an option object.

What do you think?

justLuiz commented 7 years ago

Hi Nate, I imagined something very simple, example:

liferayThemeTasks = new LiferayThemeTasks()
liferayThemeTasks.registerTasks({
  gulp: gulp
})

let myPaths = ['new1', 'new2']
let liferayPaths = liferayThemeTasks.options.sassOptions.includePaths

liferayThemeTasks.options.sassOptions.includePaths = liferayPaths.concat(myPaths)

This approach gives the user the ability to override, concatenate, or choose which of the includePaths configured by default they want or do not want to use. Also allow the use of sass-eyeglass and eyeglass-modules

robframpton commented 7 years ago

Hi guys,

I opted for allowing sassOptions to be a function that receives the default config as an argument. It's a patten I've seen in a lot of libraries and it should allow you to accomplish what you need.

'use strict';

var gulp = require('gulp');
var liferayThemeTasks = require('liferay-theme-tasks');

liferayThemeTasks.registerTasks({
    gulp: gulp,
    sassOptions: function(defaults) {
        var myPaths = ['new1', 'new2'];

        defaults.includePaths = defaults.includePaths.concat(myPaths);

        return defaults;
    }
});

Implemented in https://github.com/liferay/liferay-theme-tasks/commit/c7009351bb845ad1ecb12635c96b82bf039c4551 and will be published later today.

justLuiz commented 7 years ago

Hi @Robert-Frampton thanks, this approuch work very well, thank you.