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

[Suspense] Use style.setProperty to set display #15882

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@acdlite
Copy link
Member

commented Jun 14, 2019

Follow up to #15861.

Turns out you can't set !important using a normal property assignment. You have to use style.setProperty.

Maybe Andrew should just learn CSS.

IE9 doesn't support style.setProperty so we'll fall back to setting display: none without important, like we did before #15861. Our advice for apps that need to support IE9 will be to avoid using !important. Which seems like good advice in general, but IANACSSE.

Tested on FB and using our Suspense DOM fixture.

[Suspense] Use style.setProperty to set display
Follow up to #15861.

Turns out you can't set `!important` using a normal property assignment.
You have to use `style.setProperty`.

Maybe Andrew *should* just learn CSS.

IE9 doesn't support `style.setProperty` so we'll fall back to setting
`display: none` without `important`, like we did before #15861 Our
advice for apps that need to support IE9 will be to avoid using
`!important`. Which seems like good advice in general, but IANACSSE.

Tested on FB and using our Suspense DOM fixture.
@@ -83,25 +83,25 @@ describe('ReactDOMSuspensePlaceholder', () => {
<div ref={divs[1]}>
<AsyncText ms={500} text="B" />
</div>
<div style={{display: 'block'}} ref={divs[2]}>
<div style={{display: 'span'}} ref={divs[2]}>

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Jun 14, 2019

Member

?

Maybe you should learn some CSS.

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 14, 2019

Author Member

I changed it to span since block is the default display for divs.

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 14, 2019

Author Member

Ahaha I meant inline.

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 14, 2019

Author Member

Thanks, brain!

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 14, 2019

Author Member

I was talking to my brain, not you.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Jun 14, 2019

Contributor

This week's episode of "Andrew talks to himself" brought to you by the React team

This comment has been minimized.

Copy link
@ryota-murakami

ryota-murakami Jun 14, 2019

Contributor

But !important only accept from style.setProperty is really first time killer except holder of CSS degree😅
Thanks find out this @acdlite 😀

This comment has been minimized.

Copy link
@MuYunyun

MuYunyun Jun 15, 2019

Following up Andrew to learn CSS. 😄

@sizebot

This comment has been minimized.

Copy link

commented Jun 14, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@acdlite acdlite force-pushed the acdlite:use-style-setproperty branch from 52240a6 to 338eab2 Jun 14, 2019

@acdlite acdlite merged commit 2fe8fd2 into facebook:master Jun 14, 2019

13 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_fire Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
@NE-SmallTown

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@threepointone approved these changes 6 days ago 👀, so maybe it's not your fault Andrew(nice explanation!)

rickhanlonii added a commit to rickhanlonii/react that referenced this pull request Jun 25, 2019

[Suspense] Use style.setProperty to set display (facebook#15882)
Follow up to facebook#15861.

Turns out you can't set `!important` using a normal property assignment.
You have to use `style.setProperty`.

Maybe Andrew *should* just learn CSS.

IE9 doesn't support `style.setProperty` so we'll fall back to setting
`display: none` without `important`, like we did before facebook#15861 Our
advice for apps that need to support IE9 will be to avoid using
`!important`. Which seems like good advice in general, but IANACSSE.

Tested on FB and using our Suspense DOM fixture.
@gaearon gaearon referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.