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

Heads-up: 1.7.0 was a breaking change for us #57

Closed
gouegd opened this issue Jan 20, 2019 · 2 comments · Fixed by #58
Closed

Heads-up: 1.7.0 was a breaking change for us #57

gouegd opened this issue Jan 20, 2019 · 2 comments · Fixed by #58
Labels

Comments

@gouegd
Copy link

gouegd commented Jan 20, 2019

Hi ! I just want to share this for other people who will run into a similar issue.

We use code similar to this (simplified):

const StyledElement = styled.div`
  cursor: ${ifProp('onClick', 'pointer', 'inherit')}
`

This worked fine with 1.6.0, but in 1.7.0 we had some weird issue: our router was navigating to the wrong page, seemingly with no reason. Turns out this is because styled-tools will now call the our onClick prop (or anything that looks like a function) i n relation to this new feature: #55
So in our case, it was calling a route change with improper parameters (something like /user/undefined), simply by rendering a component.

We fixed it by reverting to the standard styled-components callback:

const StyledElement = styled.div`
  cursor: ${({ onClick }) => onClick ? 'pointer' : 'inherit'}
`

We could also have ensured the prop we use is a boolean (perhaps using an additional prop).

This may impact a few places in our code, so we're thinking of making our own simple version of ifProp that respects the legacy behaviour, so that changing our existing code is easier.

@gouegd
Copy link
Author

gouegd commented Jan 20, 2019

Just found out we have the same issue on another component.
To give you a bit of context: this is a Tag component that displays a simple tag (similar to github issue labels). Optionally clicking it may have a side effect, and in this case hovering the tag will toggle the tag's colour. That's where we typically used an ifProp on onClick, which, counting on Javascript's coercion rules, worked fine with any falsy/truthy values, including functions being present or not.

That's it :) I'm not asking for a fix or anything, I just give those details for the few others that did sometimes use styled-tools the way we do.

@diegohaz diegohaz added the bug label Jan 22, 2019
@diegohaz
Copy link
Owner

@gouegd Thank you for reporting it. That's not intended, and we should definitely fix it, even if that means reverting #56

@jtmthf Do you have any idea?

diegohaz added a commit that referenced this issue Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants