-
Notifications
You must be signed in to change notification settings - Fork 561
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
Feat(dictionary) Added new filter removePrivate
#770
Conversation
For visibility I tag @chazzmoney |
Since you were interested, I tag also @crypt096, @caseybaggz and @didoo |
I still need to improve the documentation. I'll try by myself tomorrow, but any tips are more than welcome. |
removePrivate
removePrivate
removePrivate
removePrivate
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your work on this! Just one minor thing with the example and we are good to go!
* @returns {Boolean} | ||
*/ | ||
removePrivate: function(token) { | ||
return (token && token.private) ? false : true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah so clean ✨
@@ -1,6 +1,5 @@ | |||
const fs = require('fs-extra'); | |||
const glob = require('glob'); | |||
const path = require('path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
examples/advanced/filters/README.md
Outdated
@@ -0,0 +1,99 @@ | |||
## Filters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding an example!
var filters = require('../../lib/common/filters'); | ||
|
||
describe('common', () => { | ||
describe('filters', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding unit tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! It is really nicely done.
After @dbanksdesign changes are complete, I'm ready to merge this!
Nice work.
FYI Personally I don't mind this approach, more than the previous one. Up to you! |
@silversonicaxel I don't this the filter approach is in opposition; in fact, it is a simple change to make the filter run by default if the standards body decides that is a good idea and then publish it as a breaking change. However, the final point in that discussion (that "private" may be an overloaded term with multiple meanings), is maybe a good one, which I wrote a response to here. Do we really want to use "private"? Or should we try and use a more explicit property name? |
@chazzmoney I see your point, line is subtle. there are some options on the table I think. in favour (and with a personal order preference) (I also believe that them all might be also overloaded with extra meanings)
not in favour (since it tackles the other set of design tokens to keep in, and not filter out)
What do you think? @dbanksdesign also, your opinion? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think because this is a filter and not changing core functionality and the filter name clearly says 'removePrivate' I am good with this change even if the spec goes a different route.
Description of changes:
This PR is intended to provide a new builtin filter functionality, called
removePrivate
, to allow to set a design token toprivate: true
in order to avoid to be outputted in the distribution folder after the build process.For example, having an initial source of design tokens like the following:
a potential
scss
output would be something like this:or a potential
json
output would be something like this:This PR is created as follow up of the discussion hold in this other PR #704 where the same feature was tried to be achieved with a different approach
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.