Conversation
Add new story, update styles, changed contributing guide
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 653a62d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
windows narrator would read only numbers without extra context
There was a problem hiding this comment.
Pull request overview
Updates the carousel navigation UI to use semantic <button> elements and improves related documentation/stories, aligning with issue #1081’s accessibility goals.
Changes:
- Replace nav arrow containers with
<button>elements and add aria-labels. - Update carousel nav button styling and add a Storybook story with arrows enabled.
- Refresh docs (README install commands, contributing guide) and add a changeset entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds install instructions for Yarn/NPM/PNPM. |
| packages/nuka/src/Carousel/PageIndicators.tsx | Improves screen-reader label text for dot/page indicators. |
| packages/nuka/src/Carousel/NavButtons.tsx | Switches arrow controls from div to button with aria-labels. |
| packages/nuka/src/Carousel/Carousel.stories.tsx | Adds a Storybook story enabling arrows. |
| packages/nuka/src/Carousel/Carousel.css | Updates nav button styles and visibility behavior. |
| CONTRIBUTING.md | Updates demo run instructions to the website script. |
| .changeset/hip-jobs-change.md | Adds a patch changeset for the nav element update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } |
There was a problem hiding this comment.
.nuka-nav-button declares both display: none and later display: flex, so the display: none is overridden and nav buttons will render even when they are not “enabled”. At the same time, the .nuka-nav-button-enabled rule was removed but the class is still used (and referenced by the auto-hide hover selector), so arrow visibility logic is inconsistent. Reintroduce a .nuka-nav-button-enabled rule (using display: flex) and remove the conflicting display in the base rule so enabled/disabled + hover behavior works as intended.
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .nuka-nav-button-enabled { | |
| display: flex; | |
| } |
| <button | ||
| type="button" | ||
| className={prevNavClassName} | ||
| onClick={goBack} | ||
| aria-label="Go to previous slide" | ||
| > |
There was a problem hiding this comment.
enablePrevNavButton/enableNextNavButton are computed but not reflected in the buttons’ interactive state. These buttons remain clickable/focusable even when navigation should be disabled at the ends (wrapMode="nowrap"). Consider wiring these booleans to disabled (and matching styles) and/or conditionally omitting the onClick so button semantics match the enabled state.
| Run docosaurus site on `localhost:3000` | ||
|
|
||
| ```sh | ||
| $ pnpm run start:nextjs | ||
| $ pnpm run start:website | ||
| ``` | ||
|
|
||
| To make changes to the Nuka Carousel library and have those changes reflect in the NextJS demo app also run | ||
| To make changes to the Nuka Carousel library and have those changes reflect in the Docosaurus demo app also run | ||
|
|
There was a problem hiding this comment.
“docosaurus” / “Docosaurus” appears to be a misspelling of “Docusaurus”. Updating this improves clarity and avoids propagating the typo in contributor docs.
| 'nuka-carousel': patch | ||
| --- | ||
|
|
||
| Replace div for button elements #1081 |
There was a problem hiding this comment.
The changeset summary is a bit ungrammatical (“Replace div for button elements”). Consider updating it to something like “Replace div elements with button elements (#1081)” so the release note reads cleanly.
| Replace div for button elements #1081 | |
| Replace div elements with button elements (#1081) |
Description
Add new story with arrows enabled, update styles, changed contributing guide, updated navigation to use
buttonelementsFixes #1081 (issue)
Type of Change
How Has This Been Tested?
Checklist
My code follows the style guidelines of this project (I have run
pnpm run lint)I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes (I have run
pnpm run test:ci-with-server/pnpm run test)I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have recorded any user-facing fixes or changes with
pnpm changeset.My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
I have tried to run test via
pnpm run check, but test would throw warnings with