-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Remove IE8 property expansion workaround #10803
Conversation
Yeeees 🔪🔪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming it doesn't break IE9. We could probably add a style fixture that just sets every style value and verifies its set on the node.
Looks like you need to remove the |
Does anybody want to test if it breaks IE9? If this is a one-off for IE8 I don't think it deserves a fixture. |
@gaearon it may have been included for IE8, but there's no guarantee that a fix isn't directly or indirectly preventing a break in other browsers as well, which is why I recommended a broad fixture for style properties. I say that because I thought that it had happened to me before, and it turns out it was when I tried to do this exact same thing in #9294 😄 This was breaking iOS 4 and Android 4.1 for me, so I dropped it since I wasn't sure what our browser support guidelines were (#9301) |
Based on @aweary's experience, it sounds like we need to determine if are dropping support for iOS4 Safari and Android 4.1 Browser. caniuse indicates 0% usage of iOS 4 Safari and 0.11% usage of Android 4.1 Browser globally. still, I do not know how representative this is of Facebook's audience. So do we drop these browsers? |
IIRC our guideline is > 1% of our audience. But @sophiebits can confirm whether I remember it right. |
Did some quick visual QA in IE8-11 and Android 4.2. Some of my best design work, of course: This also looked great on the Galaxy S3's native browser which makes me wonderif I'm missing a key step in QA based on @aweary's experience. But this looks good to me otherwise, 👍. |
@nhunzaker I believe I had issues with Android 4.1, not 4.2. it's also totally possible I messed up the QA last time somehow 😉 |
@aweary sorry, I missed a key detail: The Galaxy S3 I tested on was Android 4.1. I just did another check to very that styles can be toggled on or off, which, I think, covers the shorthand bug style clearing case. I've published that here: http://react-shorthand-style.surge.sh/ For whatever it's worth, this does work on Android 4.1. I think we should move forward with this change. |
We’ll wait for after 16 before landing. Since we’ll probably land a few PRs that increase the bundle size (e.g. new unstable APIs) and I want to have something to balance that. |
I verified the @nhunzaker's fixture works as expected in Android 4.1, so this LGTM whenever we want to merge 👍 |
We don’t support it anymore.
Just need to verify IE9 is not affected.