Skip to content

refactor(react): use usePrefix in Slider and SliderHandles#21678

Open
adamalston wants to merge 4 commits intocarbon-design-system:mainfrom
adamalston:n/a-after
Open

refactor(react): use usePrefix in Slider and SliderHandles#21678
adamalston wants to merge 4 commits intocarbon-design-system:mainfrom
adamalston:n/a-after

Conversation

@adamalston
Copy link
Contributor

@adamalston adamalston commented Feb 27, 2026

Partially addresses #18868

Used usePrefix in Slider and SliderHandles.

Changelog

Changed

  • Used usePrefix in Slider.
  • Used usePrefix in SliderHandles.

Testing / Reviewing

These components appeared to be the last ones in the codebase still using the PrefixContext.Consumer construct.

Existing tests should cover these changes.

yarn test --coverage \
  --runTestsByPath packages/react/src/components/Slider/__tests__/Slider-test.js \
  --collectCoverageFrom=packages/react/src/components/Slider/Slider.tsx

PR Checklist

As the author of this PR, before marking ready for review, confirm you:

  • Reviewed every line of the diff
  • Updated documentation and storybook examples
  • Wrote passing tests that cover this change
  • Addressed any impact on accessibility (a11y)
  • Tested for cross-browser consistency
  • Validated that this code is ready for review and status checks should pass

More details can be found in the pull request guide

@netlify
Copy link

netlify bot commented Feb 27, 2026

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e5b3d08
🔍 Latest deploy log https://app.netlify.com/projects/v11-carbon-react/deploys/69a4c37b298a680008589170
😎 Deploy Preview https://deploy-preview-21678--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Feb 27, 2026

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit e5b3d08
🔍 Latest deploy log https://app.netlify.com/projects/v11-carbon-web-components/deploys/69a4c37b4b9ff40008bd1ade
😎 Deploy Preview https://deploy-preview-21678--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.48%. Comparing base (0e85ce7) to head (e5b3d08).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #21678      +/-   ##
==========================================
- Coverage   87.54%   87.48%   -0.07%     
==========================================
  Files         538      538              
  Lines       43486    43483       -3     
  Branches     6651     6644       -7     
==========================================
- Hits        38071    38040      -31     
- Misses       5254     5282      +28     
  Partials      161      161              
Flag Coverage Δ
main-packages 87.40% <100.00%> (+0.03%) ⬆️
web-components 87.52% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

@adamalston it looks like the missing test coverage is isolated around twoHandles && isRtl

Not necessary to merge but a heads up if you wanted to fix.

@adamalston
Copy link
Contributor Author

adamalston commented Mar 1, 2026

I think the coverage report is highlighting unreachable code in this section:

{twoHandles ? (
<ThumbWrapper
hasTooltip={hideTextInput}
className={upperThumbWrapperClasses}
label={formatLabel(valueUpper ?? 0, undefined)}
align="top"
{...upperThumbWrapperProps}>
<div
className={upperThumbClasses}
role="slider"
tabIndex={readOnly || disabled ? undefined : 0}
aria-valuemax={max}
aria-valuemin={value}
aria-valuenow={valueUpper}
aria-label={ariaLabelInputUpper}
ref={thumbRefUpper}
onFocus={() =>
setState({ activeHandle: HandlePosition.UPPER })
}>
{twoHandles && !isRtl ? (
<>
<UpperHandle aria-label={ariaLabelInputUpper} />
<UpperHandleFocus aria-label={ariaLabelInputUpper} />
</>
) : twoHandles && isRtl ? (
<>
<LowerHandle aria-label={ariaLabelInput} />
<LowerHandleFocus aria-label={ariaLabelInput} />
</>
) : undefined}
</div>
</ThumbWrapper>
) : null}

This block is gated by twoHandles ? ... : null, so within it twoHandles is always true:

  1. There's a redundant fallback in label. If twoHandles is true, valueUpper is expected to be defined. That makes the ?? 0 fallback effectively dead code.

    label={formatLabel(valueUpper ?? 0, undefined)}

  2. The : undefined case in the ThumbWrapper children is unreachable.

    twoHandles && !isRtl ? A : twoHandles && isRtl ? B : undefined

    reduces to:

    !isRtl ? A : isRtl ? B : undefined

    Since isRtl is a boolean, the : undefined case is unreachable:

    {twoHandles && !isRtl ? (
    <>
    <UpperHandle aria-label={ariaLabelInputUpper} />
    <UpperHandleFocus aria-label={ariaLabelInputUpper} />
    </>
    ) : twoHandles && isRtl ? (
    <>
    <LowerHandle aria-label={ariaLabelInput} />
    <LowerHandleFocus aria-label={ariaLabelInput} />
    </>
    ) : undefined}

@adamalston
Copy link
Contributor Author

Re-requesting reviews since code has changed. These changes will conflict with #21673 and possibly other pull requests.

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

Labels

None yet

3 participants