Skip to content

V4 Improvements #170

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

V4 Improvements #170

wants to merge 14 commits into from

Conversation

nerdyman
Copy link
Owner

@nerdyman nerdyman commented Jun 6, 2025

Perf

  • Use CSS variables to apply current position
  • Use CSS variables to apply bounds padding
  • Remove resize observer - should be able to maintain position using CSS only

Feat

  • Make boundsPadding support any CSS unit
  • Make keyboardIncrement support any CSS unit

Checks

  • Add boundsPadding tests
  • Ensure landscape and portrait functionality works as it did before
  • Ensure keyboard increment functionality works as it did before

Copy link

vercel bot commented Jun 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compare-slider ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 22, 2025 2:44pm

@nerdyman nerdyman added the enhancement New feature or request label Jun 6, 2025
@nerdyman nerdyman marked this pull request as ready for review June 6, 2025 23:22
@nerdyman nerdyman added the publish-preview Publish a preview package label Jun 6, 2025
@nerdyman nerdyman self-assigned this Jun 6, 2025
@nerdyman nerdyman requested a review from Copilot June 6, 2025 23:29
Copilot

This comment was marked as outdated.

Copy link

pkg-pr-new bot commented Jun 6, 2025

react-compare-slider-example

npm i https://pkg.pr.new/nerdyman/react-compare-slider@170

commit: 9b50deb

BREAKING CHANGE: Unitless values are no longer supported, raw numbers like `20` must be passed as `20px`.
@nerdyman nerdyman requested a review from Copilot June 22, 2025 16:46
Copilot

This comment was marked as outdated.

@nerdyman nerdyman changed the title Feat/optimize v4 V4 Improvements Jun 24, 2025
@nerdyman nerdyman requested a review from Copilot June 25, 2025 11:06
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 refactors slider positioning to rely on CSS variables, removes the resize observer, and extends boundsPadding to accept arbitrary CSS units.

  • Replaced manual DOM clipping and resize observer with a CSS variable–based approach (currentPosition and boundsPadding).
  • Updated the boundsPadding prop from number to string, enabling any CSS unit.
  • Simplified event handling and merged imperative position updates into a single setPosition callback.

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/src/utils.ts Consolidated imports; removed useResizeObserver; adjusted hooks for undefined targets.
lib/src/types.ts Changed several interfaces to type; updated boundsPadding to string.
lib/src/consts.ts Added ReactCompareSliderCssVars for CSS variable names.
lib/src/ReactCompareSlider.tsx Rewrote position logic to use CSS vars; removed resize observer; centralized setPosition.
lib/src/Container.tsx Updated clipping to use CSS vars; removed explicit position prop on handle.
lib/src/useReactCompareSliderRef.ts Minor formatting change to console.warn.
docs/**/* Adjusted stories and README to reflect boundsPadding: string; cleaned up formatting.
.vscode/settings.json Increased ruler width to 110.
.prettierrc.js, .editorconfig Updated line length settings to 110.
.github/workflows/publish-preview.yml Bumped actions versions and simplified install command.
Comments suppressed due to low confidence (3)

lib/src/types.ts:50

  • The keyboardIncrement prop still accepts only number according to the type definitions, but the implementation now supports string units; update its type to string | number and adjust documentation accordingly.
  boundsPadding?: string;

README.md:78

  • Update the description for boundsPadding to reflect that it accepts any CSS unit (e.g., px, %, rem), not just pixels.
| [`boundsPadding`](https://react-compare-slider.vercel.app/?path=/story/demos--bounds-padding)                   | `string`                     |   | `0%`           | Padding to limit the slideable bounds in pixels on the X-axis (landscape) or Y-axis (portrait).

lib/src/Container.tsx:95

  • Removing the default aria-valuenow attribute could cause a missing initial slider value in assistive technologies. Ensure an initial aria-valuenow is set, for example by passing the initial position prop or adding a default attribute.
        aria-valuemax={100}

: (keyboardIncrement / width) * 100;
const incrementPercentage =
typeof keyboardIncrement === 'string'
? parseFloat(keyboardIncrement)
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

Parsing keyboardIncrement strings only via parseFloat treats any unit as a percentage, so passing pixel values (e.g. '20px') will not convert correctly. Consider detecting unit suffixes ('%', 'px', etc.) and converting accordingly to percent or pixel-based increments.

Suggested change
? parseFloat(keyboardIncrement)
? (() => {
const match = keyboardIncrement.match(/^([\d.]+)(px|%)?$/);
if (!match) {
console.warn('Invalid keyboardIncrement value:', keyboardIncrement);
return 0; // Default to 0 if the value is invalid
}
const [, value, unit] = match;
const numericValue = parseFloat(value);
if (unit === 'px') {
return (numericValue / width) * 100; // Convert pixels to percentage
}
return numericValue; // Assume percentage if unit is '%' or undefined
})()

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request publish-preview Publish a preview package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant