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
Collaborator

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

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.
@@ -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]}>
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Maybe you should learn some CSS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahaha I meant inline.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, brain!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking to my brain, not you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@MuYunyun MuYunyun Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up Andrew to learn CSS. 😄

@sizebot
Copy link

sizebot commented Jun 14, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@acdlite acdlite merged commit 2fe8fd2 into facebook:master Jun 14, 2019
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Jun 18, 2019

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

rickhanlonii pushed a commit to rickhanlonii/react that referenced this pull request Jun 25, 2019
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 mentioned this pull request Jul 30, 2019
This was referenced Mar 10, 2020
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.

8 participants