Skip to content

Conversation

ROWELLI
Copy link

@ROWELLI ROWELLI commented Aug 7, 2025

Summary

This PR adds a box size slider to the perf-test page and improves the animation label rendering by truncating long names to fit within the box.

Main Changes

  • Added a custom input[type=range] slider to control the width/height of each animation box
  • Styled the slider with a dark track and mint-colored thumb
  • Connected the slider to dynamically update animation size on change
  • Truncated animation names below each box
  • Adjusted the number of grid columns automatically based on the current slider-controlled box width
  • Moved the slider below the content area on small screens
slider_layout.mov

Copy link

vercel bot commented Aug 7, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
thorvg-perf-test Ready Ready Preview Comment Oct 16, 2025 3:23pm

@hermet hermet requested a review from Copilot August 7, 2025 11:40
@hermet hermet added the perf-test Performance Test label Aug 7, 2025
@hermet hermet requested a review from tinyjin August 7, 2025 11:40
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 adds a customizable box size slider to the perf-test page, allowing users to dynamically control the dimensions of animation boxes. The implementation includes visual slider styling and improves text truncation for animation labels.

  • Added an interactive range slider with custom styling to control animation box dimensions
  • Replaced static size values with dynamic state management using React hooks
  • Improved animation name display with proper text truncation using CSS ellipsis

Reviewed Changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.

File Description
perf-test/package.json Updates package dependency to use local file reference
perf-test/app/page.tsx Implements slider functionality, state management, and text truncation
perf-test/app/globals.css Adds custom CSS styling for the range slider component

@tinyjin
Copy link
Member

tinyjin commented Aug 7, 2025

@ROWELLI Hello, I cannot review due to build failure. Could you check the build?

CleanShot 2025-08-07 at 22 55 11@2x

@hermet
Copy link
Member

hermet commented Aug 11, 2025

@ROWELLI If this updates the UI or its behavior, a new ui screenshot or video would be appreciated. Thanks.

@hermet hermet added the enhancement Improve features label Aug 11, 2025
@ROWELLI
Copy link
Author

ROWELLI commented Aug 11, 2025

@ROWELLI If this updates the UI or its behavior, a new ui screenshot or video would be appreciated. Thanks.

I’ve added the video to the first comment for reference.

@hermet
Copy link
Member

hermet commented Aug 11, 2025

@ROWELLI Thanks. IMO, The current updated usage seems not very useful, since we can achieve the same result simply by adjusting the browser’s zoom level.

I think, a more practical usage would be to adjust the grid cell size along with the content, allowing more items to be packed into a row as the content gets smaller.

@tinyjin
Copy link
Member

tinyjin commented Sep 1, 2025

@ROWELLI @nunomi0 There seem to be unresolved conflicts, please resolve. Thanks.

CleanShot 2025-09-01 at 14 33 20@2x

@ROWELLI ROWELLI requested a review from tinyjin September 15, 2025 02:52
Copy link
Member

@tinyjin tinyjin left a comment

Choose a reason for hiding this comment

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

Thanks, please check comments.

@ROWELLI @nunomi0

input[type="range"].slider {
appearance: none;
-webkit-appearance: none;
-moz-appearance: none;
Copy link
Member

Choose a reason for hiding this comment

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

Since modern browsers already recognize appearance as a standard property, it’s not necessary to duplicate it with vendor prefixes. If everything works as expected, let’s simplify and keep only the standard declaration


slider.style.background = `linear-gradient(to right, #00deb5 0%, #00deb5 ${percent}%, #444 ${percent}%, #444 100%)`;
}
}, [size.width]);
Copy link
Member

Choose a reason for hiding this comment

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

size.width is already a state variable. Using useEffect only to compute derived values and manually update the DOM style is not ideal. Consider expressing this as reactive data through props or inline styles instead, so React handles the updates automatically.

Also directly manipulating the DOM style like sliderRef.style.background = ...  goes against React’s principles. This approach can conflict with React’s state-driven rendering, causing styles managed by state to be overridden or ignored. It also reduces code maintainability and makes UI updates less predictable. Instead the background can be controlled via React state or props.

Copy link

vercel bot commented Oct 15, 2025

@nunomi0 is attempting to deploy a commit to the thorvg-web Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@tinyjin tinyjin left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Let's do some final minor code cleanup.

border-radius: 50%;
border: none;
cursor: pointer;
}
Copy link
Member

Choose a reason for hiding this comment

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

Two styles look to indicate exactly the same

input[type="range"].slider::-webkit-slider-thumb,
input[type="range"].slider::-moz-range-thumb {
  appearance: none;
  width: 20px;
  height: 20px;
  background: #00deb5;
  border-radius: 50%;
  border: none;
  cursor: pointer;
}

value={size.width}
onChange={(e) => handleSliderChange(Number(e.target.value))}
className="slider"
style={sliderStyle}
Copy link
Member

Choose a reason for hiding this comment

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

style={{
    background: `linear-gradient(to right, #00deb5 0%, #00deb5 ${percent}%, #444 ${percent}%, #444 100%)`,
  }}

Please remove sliderStyle variable

const [size, setSize] = useState(isMobile ? { width: 150, height: 150 } : { width: 180, height: 180 });

const minSize = 50;
const maxSize = 180;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Constants declared inside a function are recreated on every render.
  2. Even though their values don’t change,
  3. they are repeatedly allocated and deallocated in memory.

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

Labels

enhancement Improve features perf-test Performance Test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants