feat(react-dom): add <FadeIn/>
#1362
Conversation
🦋 Changeset detectedLatest commit: 15f62f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
manudeli
requested review from
kangju2000,
bigsaigon333,
gwansikk and
SEOKKAMONI
|
Size Change: +291 B (+0.42%) Total Size: 69 kB
ℹ️ View Unchanged
|
CodSpeed Performance ReportMerging #1362 will create unknown performance changesComparing Summary
|
manudeli
force-pushed
the
react-dom/FadeIn
branch
from
c74ca38 to
d4d6c66
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1362 +/- ##
==========================================
- Coverage 72.11% 72.00% -0.12%
==========================================
Files 67 69 +2
Lines 581 593 +12
Branches 129 134 +5
==========================================
+ Hits 419 427 +8
- Misses 149 153 +4
Partials 13 13
|
|
I'm thinking about adding this implementation! I'd like to hear your thoughts. |
Co-authored-by: Juhyeok Kang <[email protected]>
|
@kangju2000 @gwansikk I'm currently thinking about changing the interface like this AS-ISWe need to manage "as" prop, Component from "as" prop must have "ref" & "style" prop const Example = () => (
<FadeIn as={Skeleton} duration={300} timingFunction="ease-in" />
)
const Skeleton = fowardRef((props, ref) => ( // We should use forwardRef
<div ref={ref} {...props} role="status" className="mb-6 animate-pulse space-y-2">
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</div>
))
// But How can we make Skeleton contain FadeIn?
// 🚨 There is no way in my opinionTO-BEI think it would be easier to understand if you use it like a children render prop, or like a register in react-hook-form. const Example = () => (
<FadeIn duration={300} timingFunction="ease-in">
{(fadeIn) => <Skeleton {...fadeIn} />}
</FadeIn>
)
const Example2 = () => (
<FadeIn duration={300} timingFunction="ease-in">
{({ ref, style }) => <Skeleton ref={ref} style={{ ..., ...style}} />}
</FadeIn>
)
const Skeleton = fowardRef((props, ref) => (
<div ref={ref} {...props} role="status" className="mb-6 animate-pulse space-y-2">
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</div>
))
// But How can we make Skeleton contain FadeIn?
const Skeleton = () => ( // We don't need to use forwardRef too
<FadeIn duration={300} timingFunction="ease-in">
{(fadeIn) => (
<div {...fadeIn} role="status" className="mb-6 animate-pulse space-y-2">
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</div>
)}
</FadeIn>
) |
manudeli
force-pushed
the
react-dom/FadeIn
branch
from
3b21542 to
7b55958
Compare
I believe a simple interface is better. The TO-BE case(like a register in react-hook-form) is easier for users to use compared to the AS-IS case, with a simpler and more user-friendly interface! (once users understand how to use it)
What kind of noise are you concerned about? Is it because of the returned fadeIn( |
|
I was thinking about the name "FadeIn" - when developers see this component name, they'd probably expect a simple wrapper that handles fade-in animation. But the current implementation seems more complex. <FadeIn duration={300} timingFunction="ease-in">
{(fadeIn) => <Skeleton {...fadeIn} />}
</FadeIn>Looking at the actual usage <FadeIn delay={200} duration={1000}>
{skeleton}
</FadeIn>
I'm curious about what specific cases require the render props pattern and "as" prop here (which adds the constraint of handling ref and style)? If there aren't any special requirements, maybe we could simplify it to a wrapper component? 😀 |
<FadeIn delay={200} duration={1000}>
{skeleton}
</FadeIn>To do this implementation, we need to make a decision to include rendering the component received as a Case: style prop Okay// AS-IS
const Skeleton = () => (
<FadeIn role="status" className="mb-6 animate-pulse space-y-2"> // contain div
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</FadeIn>
)
// TO-BE
const Skeleton = () => (
<FadeIn duration={300} timingFunction="ease-in"> // don't contain div, just give ref, style prop
{(fadeIn) => (
<div {...fadeIn} role="status" className="mb-6 animate-pulse space-y-2">
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</div>
)}
</FadeIn>
)Case: emotion css prop// AS-IS
// We can't implement emotion css prop case
// TO-BE
const Skeleton = () => (
<FadeIn duration={300} timingFunction="ease-in"> // don't contain div, just give ref, style prop
{(fadeIn) => (
<div ref={fadeIn.ref} css={fadeIn.style} role="status" className="mb-6 animate-pulse space-y-2">
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-2 w-[344px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[42px] rounded-sm bg-gray-300 dark:bg-gray-600" />
<div className="h-4 w-[34px] rounded-sm bg-gray-300 dark:bg-gray-600" />
</div>
)}
</FadeIn>
)The goal of Suspensive is to work independently of all cases, so wouldn't it be better to provide a flexible design that can be used in all cases rather than being limited to "style props" by including "div"? |
In my opinion, the usage in FadeIn is not particularly complex(In Suspensive package). There are many APIs that provide similar functionality, making it familiar and easy to learn quickly. (e.g., ErrorBoundaryGroup, SuspenseQuery, etc..)
@kangju2000 Would it be better to change it to a more appropriate name?
@manudeli I agree this point |
I agree this point |
|
@gwansikk @kangju2000 Then, could we just keep this implementation and deploy it first? If there's a better implementation, I think it's possible to fix it since @suspensive/react-dom is version 0! |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @suspensive/[email protected] ### Minor Changes - [#1362](#1362) [`244b305`](244b305) Thanks [@manudeli](https://github.com/manudeli)! - feat(react-dom): add `<FadeIn/>`
related with #1109
<FadeIn/>Influenced by blog posts such as https://tech.kakaopay.com/post/skeleton-ui-idea/, we provide a component called
<Delay/>in @suspensive/react. However, since<Delay/>blocks the rendering of children for a certain period of time, it affects the time when the<PostListSkeleton/>component is reflected on the html. Therefore, the skeleton itself can appear suddenly, and I thought this part was not good in terms of UX. Therefore, I thought that if we use a component like<FadeIn/>(name undecided), it will be reflected on the html immediately and the animation will be used to gradually display it, making it easier to implement a more natural UI.Example
PR Checklist