-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(studio): consolidate timeline editor callbacks #2688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5dd41e9
1e99115
5816e70
a9bdade
7f86018
dd77b83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { memo, useCallback, useMemo, useState } from "react"; | ||
| import { memo, useCallback, useEffect, useMemo, useRef, useState } from "react"; | ||
| import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; | ||
| import { SUPPORTED_EASES, SUPPORTED_PROPS } from "@hyperframes/core/gsap-constants"; | ||
| import { trackStudioSegmentEaseEdit } from "../../telemetry/events"; | ||
|
|
@@ -23,13 +23,17 @@ interface AnimationCardProps extends GsapAnimationEditCallbacks { | |
| animation: GsapAnimation; | ||
| defaultExpanded: boolean; | ||
| flat?: boolean; | ||
| focusedSegment?: { tweenPercentage: number } | null; | ||
| onFocusSegmentConsumed?: () => void; | ||
| } | ||
|
|
||
| // fallow-ignore-next-line complexity | ||
| export const AnimationCard = memo(function AnimationCard({ | ||
| animation, | ||
| defaultExpanded, | ||
| flat, | ||
| focusedSegment, | ||
| onFocusSegmentConsumed, | ||
| onUpdateProperty, | ||
| onUpdateMeta, | ||
| onDeleteAnimation, | ||
|
|
@@ -50,6 +54,25 @@ export const AnimationCard = memo(function AnimationCard({ | |
| const [addingProp, setAddingProp] = useState(false); | ||
| const [addingFromProp, setAddingFromProp] = useState(false); | ||
| const [expandedKfPct, setExpandedKfPct] = useState<number | null>(null); | ||
| const cardRef = useRef<HTMLDivElement>(null); | ||
| const pendingAutoScrollRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if (!focusedSegment) return; | ||
| setExpanded(true); | ||
| pendingAutoScrollRef.current = true; | ||
| setExpandedKfPct(focusedSegment.tweenPercentage); | ||
| onFocusSegmentConsumed?.(); | ||
| }, [focusedSegment, onFocusSegmentConsumed]); | ||
|
|
||
| useEffect(() => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Scroll-into-view effect queries a data attribute that no code writes cardRef.current?.querySelector<HTMLElement>(
`[data-ease-segment-pct="${expandedKfPct}"]`
)?.scrollIntoView(...)No code in the repo sets a Fix: Either add the attribute to KeyframeEaseList's segment render ( |
||
| if (!pendingAutoScrollRef.current || expandedKfPct === null) return; | ||
| const segment = cardRef.current?.querySelector<HTMLElement>( | ||
| `[data-ease-segment-pct="${expandedKfPct}"]`, | ||
| ); | ||
| segment?.scrollIntoView({ block: "nearest", behavior: "smooth" }); | ||
| pendingAutoScrollRef.current = false; | ||
| }, [expandedKfPct]); | ||
|
|
||
| const usedProps = useMemo( | ||
| () => new Set(Object.keys(animation.properties)), | ||
|
|
@@ -154,6 +177,7 @@ export const AnimationCard = memo(function AnimationCard({ | |
|
|
||
| return ( | ||
| <div | ||
| ref={cardRef} | ||
| data-flat-effect-card={flat ? "true" : undefined} | ||
| className={ | ||
| flat | ||
|
|
@@ -267,7 +291,7 @@ export const AnimationCard = memo(function AnimationCard({ | |
| onToggle={setExpandedKfPct} | ||
| onEaseCommit={(pct, ease) => { | ||
| onUpdateKeyframeEase(animation.id, pct, ease); | ||
| trackStudioSegmentEaseEdit({ ease }); | ||
| trackStudioSegmentEaseEdit({ action: "commit", ease }); | ||
| }} | ||
| onApplyAll={ | ||
| onSetAllKeyframeEases | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { ADD_METHODS, ADD_METHOD_LABELS, METHOD_TOOLTIPS } from "./gsapAnimationConstants"; | ||
|
|
||
| const STYLES = { | ||
| classic: { | ||
| method: | ||
| "rounded-lg border border-neutral-700 bg-neutral-900 px-2.5 py-1.5 text-[11px] font-medium text-neutral-300 transition-colors hover:border-neutral-600 hover:text-white", | ||
| cancel: "px-1.5 text-[11px] text-neutral-500 hover:text-neutral-300", | ||
| trigger: "text-[11px] font-medium text-neutral-400 transition-colors hover:text-neutral-200", | ||
| }, | ||
| flat: { | ||
| method: | ||
| "rounded-lg border border-panel-border-input bg-panel-input px-2.5 py-1.5 text-[11px] font-medium text-panel-text-2 transition-colors hover:border-panel-text-4 hover:text-panel-text-0", | ||
| cancel: "px-1.5 text-[11px] text-panel-text-3 hover:text-panel-text-1", | ||
| trigger: "text-[11px] font-medium text-panel-text-3 transition-colors hover:text-panel-text-1", | ||
| }, | ||
| }; | ||
|
|
||
| export function GsapAddAnimationControl({ | ||
| open, | ||
| setOpen, | ||
| onAddAnimation, | ||
| track, | ||
| variant, | ||
| }: { | ||
| open: boolean; | ||
| setOpen: (open: boolean) => void; | ||
| onAddAnimation: (method: "to" | "from" | "set" | "fromTo") => void; | ||
| track: (control: string, name: string) => void; | ||
| variant: keyof typeof STYLES; | ||
| }) { | ||
| const styles = STYLES[variant]; | ||
|
|
||
| return ( | ||
| <div className="relative pt-1"> | ||
| {open ? ( | ||
| <div className="flex gap-1.5"> | ||
| {ADD_METHODS.map((method) => ( | ||
| <button | ||
| key={method} | ||
| type="button" | ||
| title={METHOD_TOOLTIPS[method]} | ||
| onClick={() => { | ||
| track("button", `Add ${method} animation`); | ||
| onAddAnimation(method); | ||
| setOpen(false); | ||
| }} | ||
| className={styles.method} | ||
| > | ||
| {ADD_METHOD_LABELS[method] ?? method} | ||
| </button> | ||
| ))} | ||
| <button type="button" onClick={() => setOpen(false)} className={styles.cancel}> | ||
| Cancel | ||
| </button> | ||
| </div> | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setOpen(true)} | ||
| className={styles.trigger} | ||
| title="Add a new animation effect to this element" | ||
| > | ||
| + Add effect | ||
| </button> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 onFocusSegmentConsumed inline-arrow deps make scroll effect fire every render
onFocusSegmentConsumedpassed as inline arrow from both consumers (GsapAnimationSection.tsx:60 and propertyPanelFlatMotionSection.tsx:185), so identity changes every parent render. Effect at :60 lists it in deps, so effect fires every render — the!focusedSegmentguard short-circuits after the first consumption, but dependency shape is misleading.Fix: Extract with useCallback in each consumer to make the intent honest.
— Review by Rames D Jusso