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

Shorthand CSS property collision should trigger a warning #16393

Closed
rdsedmundo opened this issue Aug 14, 2019 · 5 comments · Fixed by #18002
Closed

Shorthand CSS property collision should trigger a warning #16393

rdsedmundo opened this issue Aug 14, 2019 · 5 comments · Fixed by #18002

Comments

@rdsedmundo
Copy link

I faced a "bug" today that made me spent 1h figuring out what was going on:

I'm using an external component that accepts a color prop in order to set the background-color of the root element, but this same component also accepts a background prop which I wasn't passing, and by default, it was set to ''.

The result: React didn't throw an error nor a warning, however, the resulting element in the DOM didn't contain either background-color or background, and since the element had a default background-color coming from a CSS class, it took me a while to figure out why the color that I was passing wasn't being applied, and instead it was using the one from the CSS class.

See: https://codesandbox.io/s/react-example-8rxc8

What I reported above was the static1 case.

I added other cases as a bonus, as when I was playing with this they also seemed weird to me. On static2 I define the same properties, but because I change the order, it works. On the toggleable ones, initially I can see the background, but after changing it never appears anymore.

I'm not sure if I created those extra "test cases" correctly. My main concern is really around static1 not outputting anything on the console as a warning.

Related issues:
#6348
#8689

I wonder if #14181 (@sophiebits) should have covered this?

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 17, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jan 17, 2020
@rdsedmundo
Copy link
Author

rdsedmundo commented Feb 4, 2020

@gaearon it seems that the bot automatically closed the issue, but this still needs action.

I took a time to investigate it, and I realized that the work Sophie has done covers this case, but there's just one problem: it's disabled under a feature flag. I built it locally and flipped the flag, and it works: I can see the helpful warning.

It was flagged in #14245, and as per the PR, the original intention was to unflag it on the next minor. Since then we had at least 4 new minor versions, but it is still flagged. Is there a special reason for that or was it just forgotten?

@acdlite tagging you since you added this flag, so I'm hoping you can give context here.

@sophiebits
Copy link
Collaborator

@acdlite @gaearon Can we unflag this?

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

This was released.

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

Successfully merging a pull request may close this issue.

3 participants