Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Components returned from withComponent use the wrong rootEl #376

Closed
tikotzky opened this issue Jan 18, 2018 · 8 comments
Closed

Components returned from withComponent use the wrong rootEl #376

tikotzky opened this issue Jan 18, 2018 · 8 comments

Comments

@tikotzky
Copy link
Collaborator

  • glamorous version: 4.11.3
  • glamor version: 2.20.40
  • react version: 16.2.0

Relevant code.

const A = glamorous.div({}).withComponent('a');

function Link() {
	<A href="https://www.google.com">href is missing 🙁</A>
}

What you did:
Tried to render an a tag with an href attribute

What happened:
It rendered an a tag without the href attribute

Reproduction:
https://codesandbox.io/s/vvzq1kwlw0

Problem description:
This broke in 4.11.3
Here is a codesandbox with the same code working on 4.11.2
https://codesandbox.io/s/3xr58oxqv6

Suggested solution:
Dont know the internals well enough 😄

@kentcdodds
Copy link
Contributor

Hi @tikotzky!

Thanks for the issue. It probably broke when we merged and released #374 (cc @exah).

@tikotzky, do you think you could write a unit test to reproduce this issue? Probably in this file: https://github.com/paypal/glamorous/blob/master/src/__tests__/create-glamorous.with-component.js

If you feel so inclined to provide the fix as well that would be welcome :)

Thanks!

@tikotzky
Copy link
Collaborator Author

I already have a unit test which reproduces the issue :)
I'll open a PR in a few minutes.

I'll try to fix but its my first time peering under the hood at glamorous, so might take a few...

@kentcdodds
Copy link
Contributor

Hopefully looking at #374 will give you some clues. Thanks!

@tikotzky
Copy link
Collaborator Author

I opened a PR with a failing test here #377

After a little more testing it seems like the test passes on 4.11.2, and starts failing after
d9560fa.

Judging from that it seems like a dep broke it.
I'll dig a little further to see if I can figure it out.

@kentcdodds
Copy link
Contributor

Thanks!

@tikotzky
Copy link
Collaborator Author

The test seemed to be breaking due to a change in enzyme...
I fixed the test and have a fix...
Gonna update the PR soon.

@tikotzky
Copy link
Collaborator Author

Pr updated.

Thanks for the awesome lib 😄 👍

@kentcdodds
Copy link
Contributor

Fixed!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants