-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add the possibility to set a custom root component tag #32
base: master
Are you sure you want to change the base?
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.
This is awesome, thanks so much for adding this! Just added a couple of small comments that we should address before landing, but otherwise LGTM.
Thanks again, and I'm really sorry for the delay!
```xml | ||
<span class="Typist"> Animate this text. </span> | ||
``` | ||
|
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 adding docs!
@@ -8,6 +8,7 @@ export default class Typist extends Component { | |||
|
|||
static propTypes = { | |||
children: PropTypes.node, | |||
component: PropTypes.oneOfType([PropTypes.element, PropTypes.string]), |
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.
this should be oneOfType([PropTypes.func, PropTypes.string])
, if we actually want to allow component classes.
However, I'm thinking that for simplicity's sake, we should just take a string without allowing component classes at all. I thing just specifying a dom tag should be sufficient for most use cases.
@@ -32,6 +32,7 @@ class TypistExample extends React.Component { | |||
return ( | |||
<div className="TypistExample"> | |||
<Typist | |||
component="div" |
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 really need this here?
@@ -214,9 +214,10 @@ module.exports = | |||
var isDone = this.state.isDone; | |||
|
|||
var innerTree = utils.extractTreeWithText(this.props.children, this.state.text); | |||
var WrapComponent = this.props.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.
would you mind removing this generated code from the PR? I'll make sure to generate a new dist
when I cut a new version.
@branchard, just a friendly ping :) I think you need to pull latest from master -- |
will produce: