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: use cloneElement instead of div.openByClickOn #40

Merged
merged 5 commits into from
Feb 2, 2016
Merged

fix: use cloneElement instead of div.openByClickOn #40

merged 5 commits into from
Feb 2, 2016

Conversation

imevro
Copy link
Contributor

@imevro imevro commented Jan 12, 2016

It's all because box model in CSS.

div by default holds 100% width and as you see, my toggle-portal-button actually is not a button, it's a div.

image


So this PR fixes it by using React.cloneElement for extends this.props.openByClickOn-element with onClick prop and button became a button :)

image

@tajo
Copy link
Owner

tajo commented Jan 12, 2016

This is great! I've been once trying to solve the exact same thing (but unsuccessfully). :-)

Can you please update this PR, so it's passing Travis CI? Unfortunately, I just completely changed the whole build process (check the readme/contribution or package.json scripts). The repo was also force pushed (history cleaning). Thank you!

@imevro
Copy link
Contributor Author

imevro commented Jan 12, 2016

Can you give me an idea how to fix this test?

I don't have experience with React testing, to be honest, and don't know how to use enzyme.

The test fails because now <Portal> doesn't contain button, a button is a top-level element in Portal. Probably we need fix spec for something like button is a top-level element of <Portal />?

@imevro
Copy link
Contributor Author

imevro commented Jan 13, 2016

@tajo ?

@petejodo
Copy link
Contributor

@theaqua you have a couple options. Check out the docs for enzyme. You could use the wrapper.is(selector) method possibly

@brianespinosa
Copy link

Nice. This keeps the markup more clean too. Hopefully the PR gets merged soon. Great work, @theaqua .

@imevro
Copy link
Contributor Author

imevro commented Feb 2, 2016

Hey, @tajo, I changed spec for using .text instead of .contains (https://github.com/theaqua/react-portal/commit/96044a3ba88bcbc694011f60517ec63844b8923e). Is it acceptable?

tajo added a commit that referenced this pull request Feb 2, 2016
fix: use cloneElement instead of div.openByClickOn
@tajo tajo merged commit 0e48366 into tajo:master Feb 2, 2016
@tajo
Copy link
Owner

tajo commented Feb 2, 2016

Looking good. Thanks!

@imevro imevro deleted the patch-2 branch February 2, 2016 16:29
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.

None yet

4 participants