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

Remove IE8 property expansion workaround #10803

Merged
merged 2 commits into from Sep 27, 2017
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 24, 2017

We don’t support it anymore.
Just need to verify IE9 is not affected.

@jquense
Copy link
Contributor

jquense commented Sep 24, 2017

Yeeees 🔪🔪

Copy link
Contributor

@aweary aweary left a 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.

@aweary
Copy link
Contributor

aweary commented Sep 24, 2017

Looks like you need to remove the CSSPropertyOperations and ExecutionEnvironment imports since they're unused now.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 24, 2017

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.

@aweary
Copy link
Contributor

aweary commented Sep 24, 2017

@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)

@nhunzaker
Copy link
Contributor

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?

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 24, 2017

IIRC our guideline is > 1% of our audience. But @sophiebits can confirm whether I remember it right.

@nhunzaker
Copy link
Contributor

Did some quick visual QA in IE8-11 and Android 4.2. Some of my best design work, of course:

screen shot 2017-09-24 at 9 02 02 pm

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, 👍.

@aweary
Copy link
Contributor

aweary commented Sep 25, 2017

@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 😉

@nhunzaker
Copy link
Contributor

@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.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 25, 2017

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.

@aweary
Copy link
Contributor

aweary commented Sep 25, 2017

I verified the @nhunzaker's fixture works as expected in Android 4.1, so this LGTM whenever we want to merge 👍

@gaearon gaearon mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants