search

artsy / palette

Artsy's design system
https://palette-storybook.artsy.net/
MIT License
214 stars 45 forks source link

Be defensive when animating Collapse #1316

Closed starsirius closed 1 year ago

starsirius commented 1 year ago

I noticed a flaky error happens on Force CI (example) sometimes when running tests:

TypeError: Cannot read properties of null (reading 'style')

  at style (node_modules/@artsy/palette/src/elements/Collapse/Collapse.tsx:56:26)

When running tests individually, this error doesn't happen. My theory is that the callback in setTimeout is not finished in the same test and instead is leaked into the subsequent test and fails with the null error. Perhaps the element is already gone by then.

This PR makes the callback more defensive. Not really sure how to write a test for this. 😅

dzucconi commented 1 year ago

Geniunely surprised things use this component and that it's not deprecated — it's not in the design system and there are no stories for it. Usage is going to be isolated to the order app I imagine — possible to get rid of it?

starsirius commented 1 year ago

@dzucconi Yeah you're right that only the Orders app in Force uses it. So what I can do is to port this component to Force and apply this change, and then we can deprecate and remove it from palette. Does it sound good to you?

dzucconi commented 1 year ago

@starsirius sure that sounds reasonable to me. Would be happy to get this out of Palette. But I was also wondering if there's something in the design system we could replace this with like the Expandable or something?

starsirius commented 1 year ago

I can bring it to the team and designer and see if we can be more aligned with the design system! For now, I'm going to port this over to Force and address the test flakiness. Thank you for talking through!

And interesting that how we name elements in opposite way (e.g. Expandable vs. Collapse)! :D

starsirius commented 1 year ago

Closing!