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

Allow "filter" option in config file to support array of objects #199

Closed
didoo opened this issue Nov 17, 2018 · 9 comments
Closed

Allow "filter" option in config file to support array of objects #199

didoo opened this issue Nov 17, 2018 · 9 comments

Comments

@didoo
Copy link
Contributor

didoo commented Nov 17, 2018

Calling in @elliotdickison here, since you worked on #101.
In the documentation we say that:

platform.file.filter | Function/Object (optional) | A function or object used to filter the properties that will be included in the file. If a function is provided, each property will be passed to the function and the result (true or false) will determine whether the property is included. If an object is provided, each property will be matched against the object using a partial deep comparison to determine whether the property is included.

But if the config is a JSON file, there is no way to pass a function to the filter property (maybe we should update the documentation?), so the only way to apply a filter is via an object:

"filter": {
    "attributes": {
      "category": "color"
    }
}

which in the code is transformed to a matching filter function via the Lodash matches method:

const filterFunc = _.isFunction(filter) ? filter : _.matches(filter)

This also means we can filter only for one CTI property (eg. color, or size) at the time. In reality often is useful to have a filter that can work with multiple CTI properties (eg. "give me all the color and size properties") but as is now is not possible.

What I am proposing here is to extend a little bit the capabilities of the filter, and accept an array of objects. Something like this:

"filter": [{
    "attributes": {
      "category": "size"
    }
},{
    "attributes": {
      "category": "color"
    }
}]

where the filterProperties function would look something like this (notice: the array filter function is probably wrong, I have to investigate what is the correct way to write it, but it gives you a gist of what I mean):

function filterProperties(dictionary, filter) {
  if (!filter) {
    return dictionary
  } else {
    let filterFunc;
    if (_.isFunction(filter)) {
        filterFunc = filter;
    } else if (_.isArray(filter) && filter.every(_.isObject(filter))) {
        filterFunc = function (filter) {
            return filter.some(function(obj) { return _.matches(filter); });
        };
    } else if (_.isObject(filter)) {
        filterFunc = _.matches(filter);
    } else {
        // maybe throw an error here?
    }
    return _.assign({}, dictionary, {
      allProperties: filterPropertyArray(dictionary.allProperties, filterFunc),
      properties: filterPropertyObject(dictionary.properties, filterFunc)
    })
  }
}

What do you think of this? Adding too much complexity?
(pinging @dbanksdesign + @chazzmoney if they have some ideas/comments)

@didoo
Copy link
Contributor Author

didoo commented Nov 17, 2018

Forgot to add: this is motivated by #164 (comment)

@gitMonks
Copy link

gitMonks commented Nov 20, 2018

Thanks for creating this great tool. I totally second @didoo

At the moment I am forced to split my files to achieve the multi filters. Here is how I am doing it:

"js": {
      "transformGroup": "js",
      "buildPath": "build/tokens/js/",
      "files": [
        {
          "format": "javascript/es6",
          "destination": "colors.js",
          "filter": {
            "attributes": {
              "category": "color"
            }
          }
        },
        {
          "format": "javascript/es6",
          "destination": "size.js",
          "filter": {
            "attributes": {
              "category": "size"
            }
          }
        }
      ]
    }

Moreover, the documentation should describe all the configuration options in little more detail. Is there any way I can help. Mind you I am a junior developer with very limited abilities.

@dbanksdesign
Copy link
Member

@gitMonks if there is any part of the documentation you think is lacking, confusing, or if you just have a question, feel free to file an issue or open a PR. Everyone is welcome to contribute! Better documentation is a big focus right now with 2 big PRs in the works (#164, #198). It is definitely something we need to get better at.

@didoo & all, would it make sense to add the opposite of filter, an exclude object? We could use lodash's reject method. I guess that wouldn't solve @gitMonks case... yea maybe an array might make sense. Love to hear @elliotdickison's thoughts as he is much better at JS than I am.

@chazzmoney
Copy link
Collaborator

the documentation should describe all the configuration options in little more detail

@gitMonks Would love to make the documentation better! A couple questions to help me do so:

  1. Which configuration options were you the most confused with?
  2. Which configuration options needed more detail?
  3. Do you think the additional examples in Better examples #164 would help here?

Thank you!

@chazzmoney
Copy link
Collaborator

would it make sense to add the opposite of filter, an exclude object?

Yes! Maybe we should make it work with an array and an exclude version? Maybe the exclude syntax could look something like:

"filter": {
    "exclude": {
        "category": "color"
    }
}

The only challenge I see is whether the array is considered to be AND or OR joined. The example provided by @gitMonks only makes sense as an OR. If you did both filter and exclude, it only makes sense as an AND...

This might mean we should consider a new format.

@gitMonks
Copy link

gitMonks commented Nov 22, 2018

  1. Which configuration options were you the most confused with?
  2. Which configuration options needed more detail?
  3. Do you think the additional examples in Better examples #164 would help here?

@chazzmoney I couldn't find any description of the filters. I still don't know what are the other properties I can define on filter.attributes. The closest help I found on github pages was this:

image

This needs more elaborating. To give you an idea, the examples and the boilerplate code is how I am learning to use style-dictionary.

@didoo has a gift of explaining things judging from his articles and talks. Maybe, a complete tutorial will greatly help people get started and make use of this this great tool :)

Speaking of the solution to my original issue of combining output of different filters in one file. I would think even this could solve my issue:

"filter": { "attributes": { "category": ["color", "size"] } }

Thanks again for all your work and help here!

@chazzmoney
Copy link
Collaborator

@gitMonks Super helpful info - thanks! Will see if I can get a clearer collection of info about option in #198.

@didoo
Copy link
Contributor Author

didoo commented Nov 24, 2018

Looks like this is getting complex (the multiple options, the negation, the OR vs AND). The risk is to take something that works and it's "easy" to use and convert it to something too complex and unpredictable. What do you think if instead we define a new registerFilter method in Style Dictionary (similar to what we have with registerFormat, registerTemplate, registerAction) so that we can do:

"filter": "name_of_the_filter"

(in the same way as we do "template": "name_of_template" or "format": "name_of_format").

Then in the filterProperties function declaration we can easily detect if the filter is a "custom" filter by the fact that is a string.

@chazzmoney
Copy link
Collaborator

Closing this as we have no added registerFilter in v2.7.0

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

No branches or pull requests

4 participants