-
-
Notifications
You must be signed in to change notification settings - Fork 129
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: add transform option #987
feat: add transform option #987
Conversation
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.
@hansottowirtz Thanks for opening this PR! Do you mind amending your commit message so it passes commitlint?
git commit --amend -m "feat: add transform option"
Also, could you add tests and documentation?
6ce75e5
to
33d0f9b
Compare
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.
Looks good! Could you also document this option in README.md
?
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 174 177 +3
Branches 58 60 +2
=========================================
+ Hits 174 177 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
33d0f9b
to
e76bf9a
Compare
Sorry for the many pushes, if I need to write more tests or documentation let me know :) I changed the order of arguments as well (reactNode, node, index) and added the index. |
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 addressing the comments. I left a few more minor/nitpick comments.
e76bf9a
to
259eca1
Compare
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
Motivation
My use case is as follows:
I want to wrap every element in a Wrapper. I don't think this is possible with
replace
as it "replaces" the elements, not wraps them, and I can't easily "pass through" the subtree.If you think this PR is worth considering I will follow up with tests and extra documentation.
Checklist: