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

Add registerFilter method #233

Merged
merged 7 commits into from
Jan 29, 2019
Merged

Add registerFilter method #233

merged 7 commits into from
Jan 29, 2019

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Jan 19, 2019

Issue #, if available: #207 #199

Description of changes:

  • introduced a registerFilter method for the StyleDictionary module
  • updated the filterProperties.js to receive the filter parameter only as a function
  • moved the logic to handle the case when the filter parameter is an object in the transformConfig method, and added the handling of the case where the filter is a string (supposedly a filter registered via registerFilter, otherwise throw an error)
  • updated the documentation accordingly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@didoo
Copy link
Contributor Author

didoo commented Jan 19, 2019

@dbanksdesign @chazzmoney before continuing this PR (updating tests and documentation) I need some feedback from you. Have a look at the code, it works but maybe (probably) can be better.

Things I am not 100% sure:

  1. I have moved the logic where we convert the filter from object/function to function (and added the case for the string (registered function) from the filterProperties method to the transformConfig one. The reason for that is that in filterProperties I don't have access to the StyleDictionary object (where the filters are registered). I have tried to pass it down, or to use this but without success. Do you think there would be a way to do it? Where is the "correct" location for this logic?

    const ext = {};
    if (file.filter) {
    if(typeof file.filter === 'string') {
    if (dictionary.filter[file.filter]) {
    ext.filter = dictionary.filter[file.filter];
    } else {
    throw new Error('Can\'t find filter: ' + file.filter);
    }
    } else if (typeof file.filter === 'object') {
    ext.filter = _.matches(file.filter);
    } else if (typeof file.filter === 'function') {
    ext.filter = file.filter;
    } else {
    throw new Error('Filter format not valid: ' + typeof file.filter);
    }
    }

  2. I have left a comment, about the initialization of the filter object as a property of StyleDictionary:

    this.filter = this.filter || {}; // TODO: why the other registerXyz functions don't need it? where are initialised their objects?

    I get why we need to initialise it (templates and formats are always present in SD, filter not necessarily) but I am not sure if there is a place where is better to initialise it. Any better ideas?

@dbanksdesign
Copy link
Member

For question 1, you are correct in the placement. transform/config.js is meant to 'hydrate' the config object so it has functions rather than string keys for transforms/formats/actions/etc. Adding filters, this would be the proper place to do it.

For question 2, the initialization happens in the main index.js file:
https://github.com/amzn/style-dictionary/blob/master/index.js#L36
The file that it requires returns an object. So we could just add:

filter: {}

in there and call it a day. Or do we want to include built-in filters? Maybe not if we are trying to distribute/open up the transforms/formats/actions. Sorry for stream of conscious here.

@didoo
Copy link
Contributor Author

didoo commented Jan 22, 2019

question 1

Cool, I am happy then! :)

question 2

Ahh, now I get it! Now that you have explained it, makes total sense. OK, so we have these options:

  1. we leave the "initialisation" in the lib/register/filter.js file, but is an antipattern
  2. we add a line in index.js like you suggested (with a comment why is done like this)
  3. we create an "empty" module lib/common/filters that returns an empty object, so is consistent with the others (we need to add a comment in the module anyway, and possibly also in the index.js to signal that that is a special module

Probably 2 is the simplest and clearest one. What do you think?

@dbanksdesign
Copy link
Member

I like 2 or 3. We can just go with 2, and then if we do add built-in filters in the future we can update it when that happens.

@didoo didoo changed the title [WIP] Add registerFilter method Add registerFilter method Jan 22, 2019
Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

One tiny tiny thing...

__tests__/filterProperties.test.js Outdated Show resolved Hide resolved
@didoo
Copy link
Contributor Author

didoo commented Jan 24, 2019

@dbanksdesign wait before merging, I still have to update the documentation :)

@dbanksdesign
Copy link
Member

Good call, I might have merged today by accident

@didoo
Copy link
Contributor Author

didoo commented Jan 26, 2019

@dbanksdesign I have updated the documentation files. Do you think is enough?

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@chazzmoney
Copy link
Collaborator

LGTM

@chazzmoney chazzmoney merged commit ab28dab into amzn:develop Jan 29, 2019
dbanksdesign added a commit that referenced this pull request Feb 5, 2019
* feat: add registerFilter method (#233)
* doc: remove registerTemplate from examples (#232)
* feat: add node example (#228)
* feat: making include accept an array of globs, like source does (#227)
* fix: the naming for Sass/Scss in documentation, tests, formats, fixes #213 (#222)
* fix: handle showFileHeader option in androids formats templates, fixes #237
 (#243)
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.

None yet

3 participants