Skip to content

fix[widgets][carousel]: ENG-12409 prevent dom-prop warnings and hide the empty arrows#4550

Open
floating-dynamo wants to merge 3 commits intomainfrom
ENG-12409-fix-carousel
Open

fix[widgets][carousel]: ENG-12409 prevent dom-prop warnings and hide the empty arrows#4550
floating-dynamo wants to merge 3 commits intomainfrom
ENG-12409-fix-carousel

Conversation

@floating-dynamo
Copy link
Copy Markdown
Contributor

@floating-dynamo floating-dynamo commented May 5, 2026

Description

  • The arrow buttons were plain divs passed to react-slick. Slick clones them and adds currentSlide / slideCount props, which React was complaining about that they aren't valid DOM attributes.
  • So I have now wrapped the arrows in a small CarouselArrow component that filters those out before spreading the rest onto the div.
  • Also fixed another issue, wherein setting empty content on the arrows didn't actually hide them, because slick still draws a 20x20 absolute-positioned click target.
  • Now we set arrows: false on slick when both prevButton and nextButton are empty. Anyone passing slickProps.arrows explicitly will still get the value.

Screenshot

Before

Warnings related to props
image

image

After

No warnings
image

The carousel doesn't leave invisible click areas at the edges
image


Note

Low Risk
Low risk: small, localized change to Carousel arrow rendering and react-slick configuration; main risk is minor behavior differences when consumers override slickProps.arrows.

Overview
Fixes Carousel arrow rendering to avoid React DOM warnings by wrapping react-slick arrow elements in a CarouselArrow component that strips Slick-injected props (currentSlide, slideCount).

Also disables Slick arrows entirely when both prevButton and nextButton content are empty, preventing invisible click targets from being rendered. Bumps @builder.io/widgets version to 2.1.1.

Reviewed by Cursor Bugbot for commit 91d9945. Bugbot is set up for automated code reviews on this repo. Configure here.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 5, 2026

⚠️ No Changeset found

Latest commit: 91d9945

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented May 5, 2026

View your CI Pipeline Execution ↗ for commit 91d9945

Command Status Duration Result
nx test @e2e/qwik-city ✅ Succeeded 7m 51s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 7m 4s View ↗
nx test @e2e/angular-17 ✅ Succeeded 6m 25s View ↗
nx test @e2e/nuxt ✅ Succeeded 5m 36s View ↗
nx test @e2e/gen1-remix ✅ Succeeded 5m 3s View ↗
nx test @e2e/angular-19-ssr ✅ Succeeded 5m 16s View ↗
nx test @e2e/svelte ✅ Succeeded 5m 1s View ↗
nx test @e2e/gen1-react ✅ Succeeded 4m 47s View ↗
Additional runs (38) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2026-05-07 04:02:30 UTC

@midhunadarvin
Copy link
Copy Markdown
Contributor

  • add changset before merging

Copy link
Copy Markdown
Contributor

@AishwaryaParab AishwaryaParab left a comment

Choose a reason for hiding this comment

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

LGTM

@AishwaryaParab
Copy link
Copy Markdown
Contributor

@floating-dynamo question: which sdks will this affect? both gen1 and gen2?

@floating-dynamo
Copy link
Copy Markdown
Contributor Author

@floating-dynamo question: which sdks will this affect? both gen1 and gen2?

This change is related to the widgets package.

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.

3 participants