nandorojo / moti

🐼 The React Native (+ Web) animation library, powered by Reanimated 3.
https://moti.fyi
MIT License
4k stars 127 forks source link

feat: moti skeleton style #259

Closed tyrauber closed 1 year ago

tyrauber commented 1 year ago

Instead of a height and width, use a style prop to position only the Skeleton View, allowing fine grain control over the skeleton rendering, to better match the components the skeleton is representing.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
moti ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 8, 2023 at 7:59PM (UTC)
tyrauber commented 1 year ago

@nandorojo, thank you for Moti and the Moti Skeleton!

We recently migrated a project's skeletons (a lot of views, components) to Moti Skeleton to take advantage of the REA2 performance. One thing we struggled with is getting the Skeleton positioning correct to match the children. This was primarily due to only being able to apply a height and width and not padding and margin, and the fact that styling was also applied to the children.

We developed an alternative approach, whereby you can pass a style prop and that style is applied only to the Skeleton View. This made it far easier to match the Skeleton style to the underlying component styling when converting the project.

This change should be backwards compatible, but that needs to be review. Thank you so, much.

nandorojo commented 1 year ago

there are breaking changes here that are incompatible, i suggest restarting the PR for me to review. for example, animate presence is broken. the right approach would be to forward down a style prop directly

tyrauber commented 1 year ago

Thanks for the review, @nandorojo.

Is the issue with AnimatePresence that it is below the condition? If we move it above the condition and View would it resolve the issue?

  return (
    <AnimatePresence>
      { !(show) ? (children) : (
          <View style={style}>
            {children}
          ....           

What other changes are incompatible? There is no test suite, so I am not exactly sure what the breaking changes are.

Also, what is the magic behind this line: for (let i = 0; i++, i < 3). Typescript fails validation. I believe that should be for (let i = 0; i < 3; i++), but that changes the animation dramatically in a negative way.

nandorojo commented 1 year ago

While I get the motivations here, I've used the skeleton tons of times and have managed to get this working perfectly every time. Could you possibly share a Snack of an example of layout you're trying to achieve that isn't working?

Getting the many elements working is very hard for this, and this PR isn't something I could merge, since it would break the existing skeletons in many cases.

tyrauber commented 1 year ago

Totally understand, @nandorojo.

I think the chief issue we found is that the <View> in the skeleton, continues to be rendered even after the skeleton has stopped rendering, and the stying we wanted to only apply to the skeleton, was also applied to the children as well. Which meant having to update the style on the children, to accommodate the styling on the skeleton.

After spending some time wrestling with the styling changes, on just a few elements, I looked into the code and realized that the skeleton style was being applied to the children as well, and decided a better approach for us would be to only apply the styling to the skeleton and only render the children when the skeleton finished rendering.

This gave us complete control over the styling of the skeleton, and more important the styling of the children it wrapped.

Anyway, Just wanted to share. Feel free to close this - Especially if it breaks backwards compatibility. We're comfortable running a patch for our purposes.