-
Notifications
You must be signed in to change notification settings - Fork 121
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
Lottie icons 2408 (Milestone 3) #2565
base: main
Are you sure you want to change the base?
Changes from 37 commits
82458af
f017a16
d51013a
21daac8
934fdd3
ad4ef97
a45a9f6
aba6e23
c043b76
7359a47
ff3cd33
e59e9bd
435e41c
ef16897
ae442ad
4b8b00d
ea19c46
a10cb99
3a287b9
6123396
661f1cb
8f39dcc
57e0af5
71c42b3
496e269
c67735d
30a0c0e
6ee8afd
da9058f
98b4327
a6aab79
ab9beb7
37070c0
bbba58a
dd1a309
e3bf2a4
37089f8
530d90c
9b57a5f
01de6df
f8842ab
6e80deb
014b96d
ecfc627
2311972
c4b378c
912d44a
be4d8ed
95b8ac5
cd88bd3
8e56da0
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 |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/** | ||
* AnimationKeyframe Interface. | ||
* | ||
* Defines the structure for keyframes in animations, including optional tangents. | ||
*/ | ||
interface AnimationKeyframe { | ||
i?: { x: number[]; y: number[] } // In tangents | ||
o?: { x: number[]; y: number[] } // Out tangents | ||
t: number // Time | ||
s: number[] // Values | ||
} | ||
Comment on lines
+6
to
+11
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. This is a minor point, but I recommend using JSDOC comments for property descriptions. This allows them to show up in the contextual information in VS Code Intellisense and other editors. interface AnimationKeyframe {
/** In tangents **/
i?: { x: number[]; y: number[] }
/** Out tangents **/
o?: { x: number[]; y: number[] }
/** Time **/
t: number
/** Values **/
s: number[]
} |
||
|
||
export default AnimationKeyframe |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import AnimationKeyframe from './AnimationKeyframe' | ||
import RGBA from './RGBA' | ||
|
||
/** | ||
* ColorProperty Interface. | ||
* | ||
* Defines color properties for shape items, allowing static colors, | ||
* animated colors, and keyframe animations. | ||
*/ | ||
interface ColorProperty { | ||
a: number | ||
k: RGBA | RGBA[] | number[] | AnimationKeyframe[] | ||
} | ||
|
||
export default ColorProperty |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/** | ||
* KeyframeShape Interface. | ||
* | ||
* Defines keyframe data for shapes, including in/out tangents and values. | ||
*/ | ||
interface KeyframeShape { | ||
a: number | ||
k: { | ||
i: number[][] | ||
o: number[][] | ||
v: number[][] | ||
c: boolean | ||
} | ||
} | ||
|
||
export default KeyframeShape |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import ShapeLayer from './ShapeLayer' | ||
|
||
/** | ||
* LottieData Interface. | ||
* | ||
* The main interface for Lottie data, comprising multiple layers. | ||
*/ | ||
interface LottieData { | ||
layers: ShapeLayer[] // Main layers array, which may contain various types | ||
} | ||
|
||
export default LottieData |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import KeyframeShape from './KeyframeShape' | ||
|
||
/** | ||
* NestedShape Interface. | ||
* | ||
* Represents a nested shape within a shape group, allowing for hierarchical structures. | ||
*/ | ||
interface NestedShape { | ||
ind?: number | ||
ty: string | ||
ix?: number | ||
ks?: KeyframeShape | ||
nm?: string | ||
[key: string]: unknown // Permit additional properties | ||
} | ||
|
||
export default NestedShape |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/** | ||
* RGBA Color Representation. | ||
* | ||
* Represents a color using Red, Green, Blue, and Alpha (transparency) channels. | ||
*/ | ||
type RGBA = [number, number, number, number] | ||
export default RGBA |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import ColorProperty from './ColorProperty' | ||
import KeyframeShape from './KeyframeShape' | ||
import NestedShape from './NestedShape' | ||
|
||
/** | ||
* ShapeItem Interface. | ||
* | ||
* Represents a single shape item in a Lottie animation, which can have colors, nested shapes, | ||
* keyframe animations, and additional properties. | ||
*/ | ||
interface ShapeItem { | ||
ty: string | ||
c?: ColorProperty | ||
it?: ShapeItem[] | NestedShape[] // Allow additional shape structures | ||
ks?: KeyframeShape // Optional keyframe data | ||
nm?: string | ||
[key: string]: unknown // Permit extra properties | ||
} | ||
|
||
export default ShapeItem |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import NestedShape from './NestedShape' | ||
import ShapeItem from './ShapeItem' | ||
|
||
/** | ||
* ShapeLayer Interface. | ||
* | ||
* Represents a layer that can contain multiple shapes and other properties for Lottie animations. | ||
*/ | ||
interface ShapeLayer { | ||
ty: number | ||
shapes?: (ShapeItem | NestedShape)[] // Combine both types for flexibility | ||
[key: string]: unknown // Allow for any additional properties | ||
} | ||
|
||
export default ShapeLayer |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,8 +1,12 @@ | ||||||||||
import { rgbToHex } from '@mui/material' | ||||||||||
import { useRef } from 'react' | ||||||||||
import { useSelector } from 'react-redux' | ||||||||||
import { css, cx } from '../../../styled-system/css' | ||||||||||
import { icon } from '../../../styled-system/recipes' | ||||||||||
import { token } from '../../../styled-system/tokens' | ||||||||||
import AnimatedIconType from '../../@types/AnimatedIconType' | ||||||||||
import { ICON_SCALING_FACTOR } from '../../constants' | ||||||||||
import theme from '../../selectors/theme' | ||||||||||
import LottieAnimation from './LottieAnimation' | ||||||||||
|
||||||||||
/** Animated Icon with Conditional Lottie Animation. */ | ||||||||||
|
@@ -16,11 +20,21 @@ const AnimatedIcon = ({ | |||||||||
children, | ||||||||||
animationComplete, | ||||||||||
}: AnimatedIconType) => { | ||||||||||
const isLightTheme = useSelector(state => theme(state) === 'Light') | ||||||||||
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. This is just a minor suggestion based on my opinion, not a required change.
Suggested change
It would be even nicer to make a separate selector (I use a convention of starting every selector name with
Suggested change
@raineorshine -- A little refactoring to add more selectors (and maybe switch to the 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. Thanks for the suggestion. Personally I don't love prefixing with The logic is so minimal here, that factoring out 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. I've improved AnimatedIcon by simplifying the useSelector call for isLightTheme. |
||||||||||
const lottieColor = isLightTheme ? '#000000' : '#FFFFFF' | ||||||||||
const newSize = size * ICON_SCALING_FACTOR | ||||||||||
const color = style.fill || fill || token('colors.fg') | ||||||||||
|
||||||||||
// Create a ref to the parent div | ||||||||||
const divRef = useRef<HTMLDivElement | null>(null) | ||||||||||
|
||||||||||
// Calculate dynamic color directly | ||||||||||
const computedStyle = divRef.current ? window.getComputedStyle(divRef.current) : null | ||||||||||
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.
Is there a different way to accomplish this? It's not very idiomatic in React to use the DOM as the source of truth. Is there any way we can get the desired color from the Redux state? |
||||||||||
const dynamicColor = computedStyle?.color ? rgbToHex(computedStyle.color) : lottieColor | ||||||||||
|
||||||||||
return ( | ||||||||||
<div | ||||||||||
ref={divRef} | ||||||||||
className={cx(icon(), css(cssRaw))} | ||||||||||
style={{ | ||||||||||
...style, | ||||||||||
|
@@ -30,7 +44,11 @@ const AnimatedIcon = ({ | |||||||||
display: 'inline-flex', | ||||||||||
}} | ||||||||||
> | ||||||||||
{animated ? <LottieAnimation animationData={animationData} onComplete={animationComplete} /> : children} | ||||||||||
{animated ? ( | ||||||||||
<LottieAnimation animationData={animationData || null} onComplete={animationComplete} color={dynamicColor} /> | ||||||||||
) : ( | ||||||||||
children | ||||||||||
)} | ||||||||||
</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.
Since this is only used in
AnimatedIcon.tsx
, we can move it to that module. Or even better, remove it completely and just type the props inline.