Implement carousel with basic functionality #74
Conversation
slargman
added
the
implementation task
There was a problem hiding this comment.
Looks good! I really like how you did the increment and decrement methods. the only question I had is did you try putting it on the page/testing it and making sure it works there? I didn't see you render it anywhere in the code you changed here or have any tests
|
I did put it on the page and make sure that it works as expected. I haven't written tests for it yet, but I probably should. I'm wrapping up the basic functionality on another component and then I'll come back to add those. We can let these PRs sit until they have at least one test added. |
|
sweet sounds good, no rush. |
| import React from 'react'; | ||
| import '@testing-library/jest-dom/extend-expect'; | ||
| import { fireEvent, render, screen } from '@testing-library/react'; | ||
| // import userEvent from '@testing-library/user-event'; |
There was a problem hiding this comment.
nit: unused. This doesn't really matter, but might want to put in a lint rule to prevent unused imports
There was a problem hiding this comment.
My linter is already screaming at me over so many things :(. I do have an unused imports rule, might've just ignored it. Which import can I remove? The one on line 4?
| import React, { useState } from "react"; | ||
| import PropTypes from "prop-types"; | ||
|
|
||
| function Carousel({ items, size }) { |
There was a problem hiding this comment.
Some good general style conventions, put a comment on public exports. Carousel is pretty self explanatory, but could be nice for more complicated ones, and in general for the props that are being passed
There was a problem hiding this comment.
Good call. Added some documentation. The Carousel is shared across the app, but would you suggest documentation on components that only I will be using too?
Btw displaying more than one item is for a use case like this: |
Adds a basic carousel for use across the app. Currently takes two props:
items: an array of the items that the carousel should scroll acrosssize: the number of items that the carousel should display on the screenStill to be implemented is the styling as well as a third prop
directionwhich will dictate whether the carousel should be arranged in a row or a column.Another improvement would be hiding the increment/decrement buttons at end of range states.