Skip to content

Conversation

@MaximusFT
Copy link
Contributor

I hope all my changes will be clear.
Add tests for all new cases. All test is green
image

Feel free to write in any question.

  • minor refactor
  • add option.includeFiles
  • add option.bethSymbol
  • add tests to cover new features
  • update Readme

PS: for me important to use an updated version in my project )

add option.includeFiles
add option.bethSymbol
add tests to cover new features
update Readme
@MaximusFT
Copy link
Contributor Author

MaximusFT commented Mar 9, 2021

These changes can help to close #77 issue.

@RadValentin
Copy link
Owner

I'm not sure I understand how bethSymbol is supposed to be used. To get this result .hello+.a, which is the same as .hello + .a you could just set the existing prefix param to: prefix: '.hello +'. AFAIK CSS doesn't care if there are spaces or not.

For gluing the prefix to the selector (.hello.a), the transform param can be used:

transform(prefix, selector, prefixedSelector) {
  return prefix + selector;
}

Since both the cases you presented are already covered by existing functionality then I'd avoid adding more params to the plugin.

Copy link
Owner

@RadValentin RadValentin left a comment

Choose a reason for hiding this comment

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

I'd remove bethSelector since it's covered by existing functionality.

@RadValentin
Copy link
Owner

These changes can help to close #77 issue.

That issue was referring to prefixing only certain selectors, not certain files so sadly it will remain open :(

@MaximusFT
Copy link
Contributor Author

These changes can help to close #77 issue.

That issue was referring to prefixing only certain selectors, not certain files so sadly it will remain open :(

Yes, you right... I didn't pay enough attention

@MaximusFT
Copy link
Contributor Author

Update PR.
Leave new tests and solve them as you propose.

@MaximusFT MaximusFT requested a review from RadValentin March 10, 2021 16:48
Copy link
Owner

@RadValentin RadValentin left a comment

Choose a reason for hiding this comment

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

PR looks better but you need to solve the issue I mentioned with how includeFiles works and update the tests.

rem unused test
add include tests
@MaximusFT MaximusFT requested a review from RadValentin March 11, 2021 21:25
@MaximusFT
Copy link
Contributor Author

@RadValentin
Thanks for the review.
Update PR

@MaximusFT MaximusFT changed the title Add new options includeFiles and bethSymbol Add new options includeFiles Mar 11, 2021
@RadValentin
Copy link
Owner

Looks good to me! 👍 Thanks for contributing, I'll release a new version of the plugin today.

@RadValentin RadValentin merged commit 85ff95a into RadValentin:master Mar 12, 2021
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.

2 participants