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
Member

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.

[Suspense] Use !important to hide Suspended nodes
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

This comment has been minimized.

Copy link

commented Jun 11, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

(lint failure btw ^)

@acdlite acdlite force-pushed the acdlite:important-hide branch from bc15057 to fcc3652 Jun 11, 2019

@acdlite acdlite merged commit 198ed66 into facebook:master Jun 11, 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

acdlite added a commit to acdlite/react that referenced this pull request Jun 14, 2019

[Suspense] Use style.setProperty to set display
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

[Suspense] Use style.setProperty to set display
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

[Suspense] Use style.setProperty to set display (#15882)
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 added a commit to rickhanlonii/react that referenced this pull request Jun 25, 2019

[Suspense] Use !important to hide Suspended nodes (facebook#15861)
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 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
4 participants
You can’t perform that action at this time.