Add support for AnimatePresence when using shadow DOM #2830
Conversation
tehbelinda
force-pushed
the
tehbelinda/animate-poplayout-shadowdom
branch
2 times, most recently
from
58d608b to
88dd5de
Compare
|
Hey @mattgperry, do you know what the process is to get this change merged? |
tehbelinda
force-pushed
the
tehbelinda/animate-poplayout-shadowdom
branch
from
88dd5de to
8443249
Compare
…esence with mode === `popLayout` By default this was using document.head, but that is not always available to where it's rendered, e.g. when using the shadow DOM. Instead, pass the shadow root to the new `parentDOM` prop
tehbelinda
force-pushed
the
tehbelinda/animate-poplayout-shadowdom
branch
from
8443249 to
c1f485c
Compare
for cases where it may not exist, e.g. headless tests
|
hi @mattgperry the pipeline is passing and I just updated the latest conflicts on the CHANGELOG, let me know if you are able to review and approve this or is there someone else I should ping? thanks! |
|
@kurtextrem is this something you can help with? |
tehbelinda
changed the title
popLayout
|
@SaeWooKKang @mattgperry this is a blocker for me would be huge to get this merged 🙏 |
|
Hey! I had a brief look, it looks good to me. I've forwarded the PR. |
| @@ -346,6 +346,39 @@ describe("AnimatePresence", () => { | |||
| return await expect(promise).resolves.toBe(3) | |||
| }) | |||
|
|
|||
| test("Can cycle through multiple components with mode === 'popLayout' and dom", async () => { | |||
There was a problem hiding this comment.
I'm not 100% on what this test is trying to achieve.
Is it more expected based on the changes that we should check that style blocks are being correctly added to the testDom elements?
There was a problem hiding this comment.
I was aiming for that originally but there doesn't seem to be a way to validate the style block is there, so I resorted to having something that at least exercised the prop. But now that I've added the cypress tests per the other comment, that covers it more thoroughly so I'll just remove this. Let me know if you think there should still be something here
There was a problem hiding this comment.
Thanks for the PR!
It would be good to duplicate packages/framer-motion/cypress/integration/animate-presence-pop.ts and the animate-presence-pop and dev/react/src/tests/animate-presence-pop, and rerunning all the tests in a situation that uses a new ShadowRoot.
Additionally, do we expect other elements to be used other than ShadowRoot? It feels to me like shadowRoot or root might be a more indicative prop name.
- update prop name to 'root' - update tests to exercise using shadowRoot
…poplayout-shadowdom
|
Thanks for taking a look @mattgperry ! I've added the tests as requested and updated the prop to btw as an alternative to enabling the experimental flag to test the shadow dom in cypress, I did try upgrading cypress to 5.2.0 where the behaviour is enabled by default. The tests still passed, but I was wary of mixing the upgrading of a major version in with this in case there was something else I've missed |
tehbelinda
force-pushed
the
tehbelinda/animate-poplayout-shadowdom
branch
from
7673224 to
1912a9a
Compare
By default this was using document.head, but that is not always available to where it's rendered, e.g. when using the shadow DOM. In such cases, this change allows for the shadow root to be passed to the new
parentDOMprop. This change is backwards compatible since we default todocument.headwhen not specified.Fixes #2508