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

Fix wrapper tag #272

Merged
merged 4 commits into from
Mar 23, 2017
Merged

Fix wrapper tag #272

merged 4 commits into from
Mar 23, 2017

Conversation

huumanoid
Copy link
Contributor

This PR related to #211

After applying #211, the wrapper DOM element for ReactTooltip expected to be <div> or <span>, but actually it's a <wrapper>:
screenshot from 2017-03-20 16-02-37

The reason of this behavior described by @MatanBobi here, and some more details are in c37e063 commit message.

Another issue is that we can't pass React.DOM.div to React.createElement, we should pass 'div' instead. Read more: 3c85424

Thanks @dropfen for #211 and for his concern about backward compatibility.
Thanks @MatanBobi for his report and investigation.
Thanks @wwayne for awesome ReactTooltip library.

Babel's react-jsx preset treats low-case tags as
literal elements, not as a variables.

So, for jsx:
<wrapper ... />
It generates js:
React.createElement('wrapper')

But we want:
React.createElement(wrapper)

Changing case of variable fixes the issue
React.createElement expects string or React.Component in first
argument.

React.DOM.div and React.DOM.span are neither strings nor Components.
They are wrappers around React.createElement.
Also, they are deprecated.

Read more:
https://facebook.github.io/react/docs/react-api.html#createelement

For our case, we could safely pass strings to React.createElement,
so, no need to use React.DOM.div etc
@MatanBobi
Copy link

Was planning on doing this, already forked the repo, Thanks and sorry.

@huumanoid
Copy link
Contributor Author

@MatanBobi, oh, I'm so sorry, this fix is really yours! Henceforth, let us know in advance, please! I'm sorry! I haven't noticed that you have created repository just yesterday...

@MatanBobi
Copy link

@huumanoid its not your fault at all, It was a minor fix, definitely my bad.
Thanks for the fix!
It looks good.

@huumanoid huumanoid merged commit 9399d75 into ReactTooltip:master Mar 23, 2017
@huumanoid huumanoid deleted the fix-wrapper-tag branch March 23, 2017 22:01
@huumanoid huumanoid added this to the 3.2.10 milestone Mar 27, 2017
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