Skip to content

fixing sequence stagger in generate - #278

Open
ameerf-wix wants to merge 2 commits into
masterfrom
sequence_delay_css
Open

fixing sequence stagger in generate#278
ameerf-wix wants to merge 2 commits into
masterfrom
sequence_delay_css

Conversation

@ameerf-wix

Copy link
Copy Markdown
Contributor

Description

Related Issue

Checklist

  • I have read the Contributing Guide
  • I have added/updated tests for my changes (if applicable)
  • I have updated documentation/rules/skills (if applicable)

Screenshots / Demos

Additional Notes

@ameerf-wix
ameerf-wix requested a review from ydaniv July 28, 2026 13:11
@@ -63,6 +63,10 @@ export type ResolvedEffect = ElementIdentifier &
conditions: string[];
triggerType: TimeAnimationTriggerType;
initial: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's delete this one.

triggerType: TimeAnimationTriggerType;
initial: boolean;
/** Delay in ms. For sequence effects this is the stagger base (sequenceDelay + effectOwnDelay). */
delay?: number;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we already have a delay property on effects

// element with the eased ordinal factor `f`, and CSS resolves `f * offset + base` per element.
// `base` (sequence delay + effect's own delay) and `offset` are static; only `f` is dynamic.
const staggerProp = staggerPropName(sequence.sequenceId, effect.sequenceIndex ?? 0);
const base = effect.delay || 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the + sequence.delay?

// effects its value is a `calc(...)` referencing a per-element stagger custom property that the
// run-time populates (see add.ts); otherwise it is the effect's own delay. One entry per
// css-animation keeps it positionally aligned with the `animation` list.
const delayValue = staggerCalc ?? `${effect.delay || 1}ms`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const delayValue = staggerCalc ?? `${effect.delay || 1}ms`;
const delayValue = staggerCalc ?? `${effect.delay ?? 0}ms`;

animationDeclarations.push({
name: customProps['animation-delay'],
value:
cssAnimations.map(() => delayValue).join(', ') ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the idle .map()?

Suggested change
cssAnimations.map(() => delayValue).join(', ') ||
cssAnimations.join(', ') ||

Comment on lines +484 to +493
const easingFn = _resolveOffsetEasing(sequenceConfig.offsetEasing);
const last = targets.length - 1;

fastdom.mutate(() => {
targets.forEach(({ element, prop }, ordinal) => {
// linear easing → factor === ordinal; CSS then resolves `factor * offset + base` per element
const factor = last > 0 ? easingFn(ordinal / last) * last : 0;
element.style.setProperty(prop, `${factor}`);
});
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the design doc the suggestion was to have a CSS math impl. of these easing functions, so we only need to manage setting custom properties of index, per item, and item count - perhaps on the root


// (re)write per-element stagger factors for the full matched set. Runs on initial add and on
// any list mutation (add/remove routes through addListItems → here), reindexing shifted ordinals.
_applySequenceStagger(sequenceConfig, sequenceId, sourceKey, sourceController, instance);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we want to move this lower after we have a constructed the animationGroupArgs?
Also, you're passing sourceController to have the targetController default to it, so that you can take its elements as target, but then below we have _resolveSourceElements which can also return null.
Also, looks like besides the call to Interact.addToSequence() the rest is exactly the same, so maybe simplify, remove duplication, and move this call to the bottom before the call to _attachSequenceTriggers()?

}

// (re)write per-element stagger factors for the full matched set (see _applySequenceStagger)
_applySequenceStagger(sequenceConfig, sequenceId, sourceKey!, sourceController, instance);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, perhaps we should move that to the bottom before call to _attachSequenceTriggers()?


const LIST_PROPERTY_FALLBACKS: Record<ListPropertyName, string> = {
animation: 'none',
'animation-delay': '0s',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why do we need the fallbacks? Perhaps we can just not put anything if there's nothing to add?
Also consider that if there's another animation property overriding, like in another breakpoint, it resets all longhand properties to initial values, i.e. defaults.

fastdom.mutate(() => {
targets.forEach(({ element, prop }, ordinal) => {
// linear easing → factor === ordinal; CSS then resolves `factor * offset + base` per element
const factor = last > 0 ? easingFn(ordinal / last) * last : 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, Sequence in motion already has this implemented as private. So we can check whether it makes sense to reuse this impl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants