Lottie icons 2408 (Milestone 3)

[ohrytskov]

#2408

• Scale with state.fontSize
• Preserve extended click area
• Desktop and mobile
• Dark and Light theme
• Preserve highlighting and dimming when icon is active and inactive

[trevinhofmann]

Thank you for the PR, @ohrytskov!

--

Issue 1: Icons shift during animation

Some of the animations still cause the icon to shift in place. I believe this was postponed until Milestone 3:

Screen.Recording.2024-11-13.at.12.16.57.AM.mov

---

@raineorshine -- I have a question about the "Preserve extended click area" requirement. I understand that there should be some additional clickable space around the icon, but is it expected to be this large? Currently, the entire green region is clickable, and there is no unclickable gap between the buttons:

[raineorshine]

@raineorshine -- I have a question about the "Preserve extended click area" requirement. I understand that there should be some additional clickable space around the icon, but is it expected to be this large? Currently, the entire green region is clickable, and there is no unclickable gap between the buttons:

Yes, that's right. The click region in main is correct and there should be no unclickable gap between the buttons.

[trevinhofmann]

@ohrytskov -- Just a heads up that there are some merge conflicts to resolve and puppeteer tests failing.

Please let me know if you have any questions, and let me know when you would like me to review the PR again. Thank you!

[raineorshine]

Let's properly type all Typescript code and avoid the use of any. We lose the benefits of the type checker if we use any.

[raineorshine]

changeLineColor mutates the inside of animationData without the object reference changing. This results in the awkward external key required notify the component that the value has actually changed.

Instead, use useMemo to create a derived animationDataWithColor that can be passed to Player. Then the Player component will update naturally when the animationDataWithColor reference changes. It's easier to reason about because it avoids mutations, and it's friendlier to React since it follows norms around object references.

[ohrytskov]

I’ve made a few additional updates.

[trevinhofmann]

Thank you for making those changes, @ohrytskov. The PR is looking great overall.

The shifting seems to be resolved at the default font size:

Screen.Recording.2024-11-21.at.2.53.58.AM.mov

However, there is still a shift at different font sizes.

Font size 40:

Screen.Recording.2024-11-21.at.2.55.20.AM.mov

Font size 8:

Screen.Recording.2024-11-21.at.2.56.10.AM.mov

[trevinhofmann]

Why do we need to assert the type here (and why is it safe to assume)? The type of item.c.k is RGBA | number[] | RGBA[] | AnimationKeyframe[]. How are we sure that the type here will be AnimationKeyframe[] and not potentially RGBA, number[], or RGBA[]? If we know that the type will always be AnimationKeyframe[], perhaps the type for ShapeItem needs to be updated accordingly so that we don't need to assert this type here.

[ohrytskov]

We need to assert the type because item.c.k can be different types based on whether the color property is static or animated. Earlier, ColorProperty had a broad type for k (RGBA | RGBA[] | number[] | AnimationKeyframe[]) because it was assumed there could be various types during testing and debugging. However, in this context, it's determined that when the color property is animated, item.c.k is specifically an AnimationKeyframe[]. Therefore, it's safe to assert this type here. Adjusting ColorProperty helps handle both static and animated cases appropriately, but the type assertion remains necessary because item.c.k can vary in different instances.

[trevinhofmann]

I see that the type has been narrowed a bit since that last comment, and the TS compiler assumes that the type of item.c.k is RGBA | AnimationKeyframe[]. Just trying to avoid

I'm still not sure if I understand the typing. Is it because item.ty === 'st' || item.ty === 'fl' that we can assume the type of item.c.k is AnimationKeyframe[]? If that's the case, we could update the ShapeItem definition to account for that. I'm just hoping to avoid type assertions because making these assumptions (if ever false) can lead to unexpected runtime errors that would have otherwise been caught by TS.

This isn't the only type assertion we have in the project, though, so maybe @raineorshine can give some direction on what is preferable.

[ohrytskov]

Thank you for bringing up this concern. You're absolutely right to question whether we can safely assume the type of item.c.k as AnimationKeyframe[] based on item.ty === 'st' || item.ty === 'fl'. Making unchecked assumptions can lead to unexpected runtime errors, especially if the Lottie JSON structure evolves or if we’re working with incomplete or inaccurate typings.
The current assumption is grounded in the Lottie JSON specification. For shape items of type 'st' (stroke) or 'fl' (fill), the c property generally represents color. This property can take two forms:

• A static RGBA array ([r, g, b, a]), or
• A series of animation keyframes (AnimationKeyframe[]) if the animation flag (a) is set to 1.

Thus, the conditional check (item.ty === 'st' || item.ty === 'fl') narrows the scope, making this assumption valid within this specific context. However, as you pointed out, relying on type assertions (item.c.k as AnimationKeyframe[]) can mask potential edge cases or changes in the JSON schema.

Recent Updates:

In the recent commit, I addressed this by refactoring the ColorProperty type into a discriminated union. This ensures that the type of k is correctly tied to the value of a:

type ColorProperty =
  | { a: 0; k: RGBA } // Static color
  | { a: 1; k: AnimationKeyframe[] } // Animated color

With this structure, TypeScript now guarantees that item.c.k is of type AnimationKeyframe[] whenever item.c.a === 1, eliminating the need for type assertions like item.c.k as AnimationKeyframe[].

The recent changes in the commit enhance type safety by removing unnecessary type assertions and leveraging TypeScript's capabilities. However, input from @raineorshine or other team members on the broader strategy for balancing type safety and development efficiency across the project would be valuable. While type assertions can be acceptable as a temporary solution, they should be mitigated with thorough documentation and robust test coverage. The ultimate goal is to achieve a balance between immediate development needs and long-term maintainability.

[raineorshine]

Thanks so much for the additional analysis!

Thus, the conditional check (item.ty === 'st' || item.ty === 'fl') narrows the scope, making this assumption valid within this specific context. However, as you pointed out, relying on type assertions (item.c.k as AnimationKeyframe[]) can mask potential edge cases or changes in the JSON schema.

I think type predicates might give you the best of both worlds here. The type assertions can be tightly coupled to the relevant condition, and Typescript does the narrowing for you.

[ohrytskov]

PR updated and ready for the next review.

[trevinhofmann]

Thank you, @ohrytskov. The italic button seems to have the correct scaling at each font size. Some of the other buttons still have minor scaling issues, though. Below are a few examples.

The strikethrough animation is slightly smaller than the icon, so it shifts to a larger size when the animation completes (default font size):

Screen.Recording.2024-11-21.at.6.23.44.PM.mov

The swap parent animation is slightly larger than the icon, so it shifts to a smaller size when the animation completes (default font size):

Screen.Recording.2024-11-21.at.6.26.35.PM.mov

The subcategorize animation is slightly taller than the icon, so it shortens when the animation completes (default font size):

Screen.Recording.2024-11-21.at.6.28.28.PM.mov

The pin icon shifts up and to the left after the animation, at a font size of 40 (may also happen at the default font size, but not as noticeable):

Screen.Recording.2024-11-21.at.6.31.54.PM.mov

There may be other icons affected as well. Can you please retest each animation at the minimum (8), default (18), and maximum (40) font size to check for scaling inconsistencies before the next review?

---

Somewhat unrelated, but I've also noticed that the (inactive->active) animations are playing when no thought is selected. Is that the intended behavior, @raineorshine?

If not, I would recommend addressing it in a separate PR as it is unrelated to Milestone 3.

Screen.Recording.2024-11-21.at.6.35.10.PM.mov

[raineorshine]

Somewhat unrelated, but I've also noticed that the (inactive->active) animations are playing when no thought is selected. Is that the intended behavior, @raineorshine?

No, that's not intentional. The lottie animation should not play if the command is not executed successfully. This generally is prevented when canExecute returns false, though in some cases canExecute can return true and exec can return early.

If not, I would recommend addressing it in a separate PR as it is unrelated to Milestone 3.

I think this needs to happen as part of this PR, otherwise it will introduce a regression into main.

There may be other icons affected as well. Can you please retest each animation at the minimum (8), default (18), and maximum (40) font size to check for scaling inconsistencies before the next review?

Yes, I'd like to second this. Testing each icon at different font sizes is an important step before requesting another review. Addressing those proactively will save us some back-and-forth in the review process. Thanks so much for your careful attention to detail. (I will add a bonus for this milestone as a way of saying thanks for your additional time 🙏)

[ohrytskov]

@trevinhofmann, the updated version of the PR is ready for review.