Introduce a customizable loading stagger animation on hypertableV2 - #275
Conversation
| background-color: white; | ||
| } | ||
|
|
||
| // Apply stacking fixes only on cells that actually get the custom effect class. |
There was a problem hiding this comment.
left this here for context, it's in the dummy app so 🤷
Can also remove if needed 😇
| get disableInitialRowsAnimationExtraEffectOnSelectionCells(): boolean { | ||
| return !this.initialRowsAnimation?.includeSelectionColumnInExtraEffect; | ||
| } |
There was a problem hiding this comment.
the default seems to be false, so maybe this should be the contrary ? (make it special when it's enabled ?)
There was a problem hiding this comment.
applied a change, maybe I went too far in the inversion of logic ^^ let me know :P
| } as const satisfies Omit<InitialLoadAnimationConfig, 'extraColumnCellEffectClass' | 'columns'>; | ||
| } as const satisfies Omit<InitialLoadAnimationConfig, 'extraColumnEffect' | 'includeSelectionColumnInExtraEffect'> & { | ||
| extraColumnEffect: Pick<InitialLoadAnimationExtraColumnEffect, 'delayMs'>; | ||
| includeSelectionColumnInExtraEffect: boolean; |
There was a problem hiding this comment.
nit: I would probably have picked (Pick<>) but no matter, this type is already getting out of control 😅
|
|
||
| if (!option) return null; | ||
|
|
||
| const options: InitialLoadAnimationOption = option === true ? {} : option; |
There was a problem hiding this comment.
question: The typing here threw me a little. Shouldn't it be Partial<InitialLoadAnimationConfig> like the option?
Also naming... option => options: I find it a little easy to confuse the two. Maybe a more evocative name, wdyt?
There was a problem hiding this comment.
I have removed the typing here so that is is infered directly
| ...options, | ||
| extraColumnEffect: { | ||
| ...DEFAULT_INITIAL_LOAD_ANIMATION_CONFIG.extraColumnEffect, | ||
| ...options.extraColumnEffect |
There was a problem hiding this comment.
issue: ...undefined is going to throw I think.
| ...options.extraColumnEffect | |
| ...(options?.extraColumnEffect ?? {}) |
There was a problem hiding this comment.
undefined case is already handled & early returned on line 141
| this.initialLoadAnimationActive = true; | ||
|
|
||
| const rowsAnimationWindowMs = Math.max(this.args.handler.rows.length - 1, 0) * this.initialLoadAnimation.staggerMs; | ||
| const activeDurationMs = |
There was a problem hiding this comment.
AIssue: Keep the animation context active through a configured extra-effect delay.
activeDurationMs omits extraColumnEffect.delayMs, while each cell schedules that class after the row delay plus that extra delay. A valid configuration such as maxAnimationDurationMs: 100 with extraColumnEffect.delayMs: 500 disables the context after 100ms, cancels the cell timer in resetExtraEffectState(), and the requested class is never applied. Include the maximum extra-effect delay in the active duration, or explicitly validate/document that maxAnimationDurationMs must exceed it. Add an integration test that asserts a delayed extra class actually appears.
There was a problem hiding this comment.
yup, that is wanted on my part.
maxAnimationDurationMs is an overall stop including the extraEffects
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
5 participants
What does this PR do?
This PR adds an initial-load animation system to HyperTableV2:
OptionSetwith a newinitialRowsAnimationconfiguration objectDocumentation has been updated, you can check there to have more details of how it's used.
Good PR checklist