Skip to content

experimental: Animation axis Icons change #5060

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

Merged
merged 3 commits into from
Mar 27, 2025
Merged

experimental: Animation axis Icons change #5060

merged 3 commits into from
Mar 27, 2025

Conversation

istarkov
Copy link
Member

@istarkov istarkov commented Mar 27, 2025

Description

image

Steps for reproduction

  1. click button
  2. expect xyz

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 0000)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@istarkov istarkov requested review from Copilot and TrySound March 27, 2025 05:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR experiments with changing the animation axis icons by updating the icon imports and modifying how axis values are handled.

  • Updated imported icons from Repeat* to Arrow* to reflect new visual design.
  • Modified the axis value conversion by adding the convertAxisToXY helper function and updating the ToggleGroup’s value mapping.
Comments suppressed due to low confidence (1)

apps/builder/app/builder/features/settings-panel/props-section/animation/animation-section.tsx:28

  • The toast error message text "Schemas are incompatible, try fix" is unclear; consider rewording it to something like "Invalid animation schema. Please check the provided value."
import { ArrowDownIcon, ArrowRightIcon } from "@webstudio-is/icons";

@istarkov istarkov requested a review from Copilot March 27, 2025 05:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the animation axis icons and the handling of axis values by replacing the previous repeat icons with arrow icons and modifying the axis conversion logic.

  • Replaces RepeatColumnIcon and RepeatRowIcon with ArrowDownIcon and ArrowRightIcon.
  • Removes direct support for "block" and "inline" axes by excluding them from the axis description and adding a conversion function.
  • Updates the ToggleGroup component to use the normalized axis values.
Comments suppressed due to low confidence (2)

apps/builder/app/builder/features/settings-panel/props-section/animation/animation-section.tsx:89

  • [nitpick] The function name 'convertAxisToXY' may be confusing since it is used to normalize values that may already be 'x' or 'y'. Consider renaming it to 'normalizeAxis' for better clarity.
const convertAxisToXY = (axis: NonNullable<AnimationAction["axis"]) => {

apps/builder/app/builder/features/settings-panel/props-section/animation/animation-section.tsx:305

  • Double-check if applying 'convertAxisToXY' in the onValueChange handler is necessary when the ToggleGroup only provides 'x' or 'y' values. Removing redundant conversion could simplify the logic.
onValueChange={(axis: keyof typeof animationAxisDescription) => { handleChange({ ...value, axis: convertAxisToXY(axis) }, false); }}

@istarkov istarkov requested a review from Copilot March 27, 2025 05:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the animation axis icons and refactors the axis mapping logic to remove support for block and inline axis values.

  • Changed icon imports to use ArrowDownIcon and ArrowRightIcon.
  • Updated the animationAxisDescription to only support x and y axes and added a conversion function to map legacy "block"/"inline" values.
  • Modified the error message for schema incompatibility and adjusted the ToggleGroup's value and onValueChange handlers accordingly.
Comments suppressed due to low confidence (2)

apps/builder/app/builder/features/settings-panel/props-section/animation/animation-section.tsx:89

  • [nitpick] Consider renaming convertAxisToXY to a name like normalizeAxis to clearly indicate that the function standardizes the axis values.
const convertAxisToXY = (axis: NonNullable<AnimationAction["axis"]) => {

apps/builder/app/builder/features/settings-panel/props-section/animation/animation-section.tsx:304

  • [nitpick] The conversion logic is applied both when setting the ToggleGroup value and again in the onValueChange callback. If intentional, please add a clarifying comment; otherwise, consider consolidating the conversion to avoid potential redundant processing.
value={convertAxisToXY(value.axis ?? ("y" as const))}

@istarkov istarkov merged commit 678dde8 into main Mar 27, 2025
17 checks passed
@istarkov istarkov deleted the axis branch March 27, 2025 06:55
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.

1 participant