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

feat: add transform option #987

Merged

Conversation

hansottowirtz
Copy link
Contributor

@hansottowirtz hansottowirtz commented Jul 20, 2023

Motivation

My use case is as follows:

function Wrapper({ node, children }) {
  const [show, setShow] = useState(false)
  
  return <>
    {show && children}
    <button onClick={() => setShow(s => !s)}>Toggle</button>
  </>
}

domToReact(nodes, {
  transform(node, reactNode) {
    return <Wrapper node={node}>{reactNode}</Wrapper>
  }
})

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:

@remarkablemark remarkablemark self-assigned this Jul 20, 2023
@remarkablemark remarkablemark added the feature New feature or request label Jul 20, 2023
Copy link
Owner

@remarkablemark remarkablemark left a 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?

@remarkablemark remarkablemark changed the title Add transform option feat: add transform option Jul 20, 2023
Copy link
Owner

@remarkablemark remarkablemark left a 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
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #987 (e76bf9a) into master (3c9c777) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e76bf9a differs from pull request most recent head 259eca1. Consider uploading reports for the commit 259eca1 to get more accurate results

@@            Coverage Diff            @@
##            master      #987   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          174       177    +3     
  Branches        58        60    +2     
=========================================
+ Hits           174       177    +3     
Impacted Files Coverage Δ
lib/dom-to-react.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hansottowirtz
Copy link
Contributor Author

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.

lib/dom-to-react.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
remarkablemark
remarkablemark previously approved these changes Jul 20, 2023
Copy link
Owner

@remarkablemark remarkablemark left a 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.

Copy link
Owner

@remarkablemark remarkablemark left a comment

Choose a reason for hiding this comment

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

LGTM

@remarkablemark remarkablemark merged commit 725acc2 into remarkablemark:master Jul 20, 2023
4 checks passed
@remarkablemark
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants