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

Dynamic wrapper #211

Merged
merged 3 commits into from Feb 7, 2017
Merged

Dynamic wrapper #211

merged 3 commits into from Feb 7, 2017

Conversation

ghost
Copy link

@ghost ghost commented Oct 19, 2016

Adds a new prop wrapper.
default: "div"
supportedWrappers: span, div

Let the user define the wrapper for the Tooltip dynamically.
fixes -> #210

@wwayne
Copy link
Collaborator

wwayne commented Nov 23, 2016

I was thinking why don't just use <span> directly, I mean maybe I should use span as default wrapper, would it be better?

@ghost
Copy link
Author

ghost commented Nov 23, 2016

Well, yeah you can use span as the default, but it could mean some upgrading issues, for people who are rendering divs inside the tooltip in production. It's safer to keep div as the default.

Only span would be not better, because if you render blocks (div) inside inline (span) elements you'll get an error like #210

To have a choice is better ;)

wwayne added a commit that referenced this pull request Feb 7, 2017
@wwayne wwayne merged commit f39cac9 into ReactTooltip:master Feb 7, 2017
@MatanBobi
Copy link

MatanBobi commented Mar 16, 2017

After reviewing this PR and its result,
I keep getting "wrapper" elements inside my html.
I think that its because of this:

https://github.com/dropfen/react-tooltip/blob/25aa42a2b8113aff21bb053f309d7ace9b937f93/src/index.js#L428

React doesnt render the variable wrapper as a div/span but as a DOM element.
I think its because that the variable isnt with capital letter so react doesnt see it as a component.

@huumanoid
Copy link
Contributor

huumanoid commented Mar 16, 2017

Yes, you are right! Thank you for your review!
Babel's react preset treats wrapper as a tag name, not as a component.
https://github.com/wwayne/react-tooltip/blob/master/standalone/react-tooltip.js#L843

If you change wrapper to Wrapper, everything is fine.
Could you make a PR, please?

Also, take a note that there is a linter error.

~/github/react-tooltip/src/index.js
  441:9  error  Empty components are self-closing  react/self-closing-comp

@huumanoid huumanoid mentioned this pull request Mar 20, 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.

3 participants