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 !important to hide Suspended nodes #15861

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 11, 2019

Suspended nodes are hidden using an inline display: none style. We do this instead of removing the nodes from the DOM so that their state is preserved when they are shown again.

Inline styles have the greatest specificity, but they are superseded by !important. To prevent an external style from overriding React's, this commit changes the hidden style to display: none !important.

MaYBE AnDREw sHOulD JusT LEArn Css

I attempted to write a unit test using getComputedStyle but JSDOM doesn't respect !important. I think our existing tests are sufficient but if we were to decide we need something more robust, I would set up an e2e test.

Suspended nodes are hidden using an inline `display: none` style. We do
this instead of removing the nodes from the DOM so that their state is
preserved when they are shown again.

Inline styles have the greatest specificity, but they are superseded by
`!important`. To prevent an external style from overriding React's, this
commit changes the hidden style to `display: none !important`.

MaYBE AnDREw sHOulD JusT LEArn Css

I attempted to write a unit test using `getComputedStyle` but JSDOM
doesn't respect `!important`. I think our existing tests are sufficient
but if we were to decide we need something more robust, I would set up
an e2e test.
@sizebot
Copy link

sizebot commented Jun 11, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@threepointone
Copy link
Contributor

(lint failure btw ^)

@acdlite acdlite merged commit 198ed66 into facebook:master Jun 11, 2019
acdlite added a commit to acdlite/react that referenced this pull request Jun 14, 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.
acdlite added a commit to acdlite/react that referenced this pull request Jun 14, 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.
acdlite added a commit that referenced this pull request 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.
rickhanlonii pushed a commit to rickhanlonii/react that referenced this pull request Jun 25, 2019
Suspended nodes are hidden using an inline `display: none` style. We do
this instead of removing the nodes from the DOM so that their state is
preserved when they are shown again.

Inline styles have the greatest specificity, but they are superseded by
`!important`. To prevent an external style from overriding React's, this
commit changes the hidden style to `display: none !important`.

MaYBE AnDREw sHOulD JusT LEArn Css

I attempted to write a unit test using `getComputedStyle` but JSDOM
doesn't respect `!important`. I think our existing tests are sufficient
but if we were to decide we need something more robust, I would set up
an e2e test.
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.

4 participants