-
Notifications
You must be signed in to change notification settings - Fork 433
Carousel: Make controlled component #1974
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
Carousel: Make controlled component #1974
Conversation
…to pr/kevinparkerson/1974
interactivellama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor questions/comments.
| style={ | ||
| Object { | ||
| "transform": "translateX(0px)", | ||
| "transform": "translateX(NaNpx)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NaNpx seems concerning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these are the result of code within canUseDOM / canUseEventListeners conditionals not running. There are actually 56 instances of this in the snapshot, and each one is linked to numbers determined by the stageWidth DOM measurement. Since browser tests and testing the component manually show there is no issue, I think these can be ignored.
components/carousel/index.jsx
Outdated
| ]), | ||
| /** | ||
| * Boolean showing whether the autoplay feature is available or not | ||
| * Dictates the currently active/visible carousel panel. 1-indexed. Use with `onRequestPanelChange` for a controlled carousel component. If not provided, the carousel will manage this itself via state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I know of anything else 1-indexed in the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to 0 indexing
…rson/design-system-react into carousel-controlled-component
|
Merged! Do you think you could write a jQuery version of this Carousel next week? |
|
🤣 |
Fixes #1958
NOTE: MERGE #1973 FIRST
Just a few notes with this one:
hasAutoplayprop. The assistive text changed toautoplayButtonwhen doing this, otherwise nothing else changed API-wise. Capitalizing the P in autoplay would have changed much more of the API (though this is a prototype component so does it matter?)onRequestPanelChangeoronPanelChange? Same foronRequestAutoplayToggle. I wasn't sure but for some reasononRequestPanelChangefeels more correctisInfinitewas an available prop that was supposed to control whether the carousel can loop back to the first or last panel while navigating. It wasn't working properly, but I have now fixed it so it behaves at it should. Additionally, I made autoplay either stop or loop depending on what the value of that prop is. Would you agree that is appropriate behavior?indicatorStylesprop as it did nothing and seemed pointlessCONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).REVIEWER checklist (do not remove)
components/component-docs.jsontests.Required only if there are markup / UX changes
last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.