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

Allows preserving children from whitelistPatterns #58

Merged
merged 7 commits into from Mar 20, 2018
Merged

Allows preserving children from whitelistPatterns #58

merged 7 commits into from Mar 20, 2018

Conversation

cmizzi
Copy link
Contributor

@cmizzi cmizzi commented Feb 26, 2018

Proposed changes

Related to #39

Types of changes

  • Add ability to provide a whitelistPatterns object instead of RegExp in order to resolve a keepChildren option to preserve left to right reader

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • Assert Flow is working as well
  • I have added necessary documentation (if appropriate)

@Ffloriel Ffloriel self-requested a review March 11, 2018 12:06
@Ffloriel
Copy link
Member

Ffloriel commented Mar 12, 2018

Thanks for the PR.
I think it's important to discuss about the API itself.
From the PR, if I understand correctly, someone would whitelist patterns the following way:

whitelistPatterns: [
                {
                    pattern: /^double-class$/,
                    keepChildren: true
                }
            ]

I think it would be better to have a different property for whitelist patterns.
The list of patterns can be long depending on the use cases and it could be easier to configure with a different property:

whitelistPatterns: [
                {
                    pattern: /^single-class$/,
                    keepChildren: true
                },
                {
                    pattern: /^double-class$/,
                    keepChildren: true
                },
                {
                    pattern: /^triple-class$/,
                    keepChildren: false
                },
                {
                    pattern: /^quadruple-class$/,
                    keepChildren: false
                },
                {
                    pattern: /^quintuple-class$/,
                    keepChildren: true
                },
            
            ]

vs

whitelistPatterns: [/^triple-class$/, /^quadruple-class$/],
whitelistPatternsChildren: [/^single-class$/, /^double-class$/, /^quintuple-class$/]

@cmizzi @jsnanigans, what do you think about this? I'm not sure whitelistPatternsChildren is the best name for it but we can come up with something better.

@cmizzi
Copy link
Contributor Author

cmizzi commented Mar 12, 2018

Great idea. But, what happen if both contain same value ? We need a priority manager. First, we should manage whitelistPatternsChildren if fails, tests with whitelistPatterns

@Ffloriel
Copy link
Member

In my opinion, the priority should go to the most conservative option. WhitelistPattersChildren will conserve children selectors. If case of a misconfiguration, giving priority to this option will not remove wanted css and break the application style.
So if both contain the same value, WhitelistPattersChildren will have the priority.

@cmizzi
Copy link
Contributor Author

cmizzi commented Mar 14, 2018

I've applied changes. Now, a whitelistPatternsChildren is defined and works as excepted

add whitelistPatternsChildren to typescript definition file
Copy link
Member

@Ffloriel Ffloriel left a comment

Choose a reason for hiding this comment

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

LGTM

@Ffloriel Ffloriel merged commit f25921e into FullHuman:master Mar 20, 2018
@Ffloriel
Copy link
Member

Thanks for the PR. The option is now available with v0.21.0

@robertvrabel
Copy link

Thank you for this PR! Looking forward to trying it out :)

Ffloriel pushed a commit that referenced this pull request Oct 30, 2019
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