-
Notifications
You must be signed in to change notification settings - Fork 19
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
Initial README documentation #9
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.
left a few notes, but overall this lgtm!
README.md
Outdated
|
||
const xmlToReact = new XMLToReact({ | ||
Example: (attrs) => ({ type: 'ul', props: attrs }), | ||
Item: (attrs) => ({ type: 'li', props: attrs }) |
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.
should we show an example where type
is a custom React component?
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.
yes, I was a tad lazy
README.md
Outdated
</Example> | ||
`); | ||
|
||
React.render('app-container', reactTree); |
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.
nit: do we mean ReactDOM
instead of React
here?
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.
yep 😆
|
||
Converters are required mapping functions that define how an XML node should be converted to React. A converter must return an object in the format `{ type, [props] }`, which is intended to be passed to [`React.createElement`](https://reactjs.org/docs/react-api.html#createelement). | ||
|
||
- `type` - required tagName, React component, or React fragment |
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.
💯
README.md
Outdated
Converters are required mapping functions that define how an XML node should be converted to React. A converter must return an object in the format `{ type, [props] }`, which is intended to be passed to [`React.createElement`](https://reactjs.org/docs/react-api.html#createelement). | ||
|
||
- `type` - required tagName, React component, or React fragment | ||
- `props` - optional props object |
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.
could we place "optional" in parenthesis, similar to below?
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.
👍
README.md
Outdated
|
||
```js | ||
{ | ||
[nodeName]: converterFunction |
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.
nit: could we remove the brackets around nodeName
? it reads as being optional
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.
oh, yeah, i was using the dynamic property syntax. but yes, i'll make this change
This is still work in progress
After spending time working on examples and the API documentation, I realized that I probably should have just started with the generic project info. Maybe I can split this apart.