Feedback

[bebraw]

Hi,

Here's feedback as promised. I know you are probably aware a lot of these but just listing them out as I go:

• package.json - Fix author
• package.json - Fix repository url
• package.json - Add a build script - just "build": "webpack" should work at the scripts section. In addition it can be a good idea to set up a development build. You can see some basic ideas at my boilerplate. You can automate this so that build gets generated only when you trigger a versioning script. Least effort that way.
• package.json - peerDependencies - I would add an upper limit of < 1.0.0 for React. No doubt the API will change somehow but current will work as well.
• webpack - Consider using babel-loader instead of jsx-loader. You get JSX and a lot more. It's well maintained and develops fast
• demo - If I load the page to browser and hit right once, the whole carousel goes around once and returns to the same item. After that it behaves as I would expect it to. You can see the behavior also if you go left. It's always the first operation that triggers the behavior. Could there be something wrong with indexing?
• demo - I get a 404 on the initial image. See attachment.
• demo - The prev/next buttons look a bit tacky. Might as well replace them with something neat and simple. :)
• demo - It might be a nice idea to stash the settings in localStorage so that the settings get retained. Here's a rough idea. You can do something like that.
• Mainview.jsx - I would ditch the panel-count id. This would probably mean ditching the table too as if you want to associate a label with a control some other way, the label has to contain the control.
• Carousel.jsx - Same. I would drop carousel id and replace that with a class. id will become problematic when you have many carousels on the same page.
• Carousel.jsx - Eliminate direct dependency on Images and inject instead
• Carousel.jsx - It feels like it could be a good idea to extract some, if not all, of the state. I like to give control over state to the user as you never know how they want to treat it. See Controllable React Components for some discussion on the topic.
• Carousel.jsx - It could be nice to allow the user to replace Layout with something completely custom. As long as you define a standard interface the passed Layout should conform with, this should be nice.
• Carousel.jsx - Maybe same makes sense for easing. I can see you have a very nice collection of defaults, though.

Overall it's a cool little component! I would fix at least that indexing behavior. Rest is up to you.

[bobiblazeski]

Great feedback . The only problem that I'm stumbling upon is the state, will take a look at suggested resource to get some idea.

[bobiblazeski]

I've extracted few comments as separate issues.

Fixed below issues:

package.json - Fix author

Done

package.json - Fix repository url

Done

package.json - peerDependencies - I would add an upper limit of < 1.0.0 for React. No doubt the API will change somehow but current will work as well.

Done

webpack - Consider using babel-loader instead of jsx-loader. You get JSX and a lot more. It's well maintained and develops fast

Good tip, done. Will use babel in my other React projects.

demo - If I load the page to browser and hit right once, the whole carousel goes around once and returns to the same item. After that it behaves as I would expect it to. You can see the behavior also if you go left. It's always the first operation that triggers the behavior. Could there be something wrong with indexing?

It was a bug since I added angles to state twice. Fixed.

demo - I get a 404 on the initial image. See attachment.

Fixed url.

Carousel.jsx - Same. I would drop carousel id and replace that with a class. id will become problematic when you have many carousels on the same page. Fixed used classes. Carousel.jsx - Eliminate direct dependency on Images and inject instead Fixed images are sent as array of sources.

Regarding the remaining comments:

Carousel.jsx - It could be nice to allow the user to replace Layout with something completely custom. As long as you define a standard interface the passed Layout should conform with, this should be nice.

I believe that layout is the core of the carousel control, if someone creates other types of layout it should be added in this project since its tied to functionality of the carousel. Here's two more layout that I want to add when time permits Star topology http://www.webdesign-flash.ro/p/u3dcar/index-minimal-classic.html & Royal topology http://www.webdesign-flash.ro/p/royal-3d-carousel/index.html

demo - It might be a nice idea to stash the settings in localStorage so that the settings get retained. Here's a rough idea. You can do something like that. Mainview.jsx - I would ditch the panel-count id. This would probably mean ditching the table too as if you want to associate a label with a control some other way, the label has to contain the control.

I believe that this is something better left for the user. Though demo needs more work, with carousel sample at minimum. Will improve upon this later.

Carousel.jsx - It feels like it could be a good idea to extract some, if not all, of the state. I like to give control over state to the user as you never know how they want to treat it. See Controllable React Components for some discussion on the topic.

This is as part where I completely don't understand, what do you suggest. The state stored in depo is something that I don't think makes sense to extract since it stores the intermediate state while the carousel is animating.

[bebraw]

Star topology http://www.webdesign-flash.ro/p/u3dcar/index-minimal-classic.html & Royal topology http://www.webdesign-flash.ro/p/royal-3d-carousel/index.html

Very nice!

This is as part where I completely don't understand, what do you suggest. The state stored in depo is something that I don't think makes sense to extract since it stores the intermediate state while the carousel is animating.

Yeah, it's true there probably must be some state like that. I recall the idea with the comment was that you never know how users will want to use your components. Therefore maintaining minimal amount of state can be a nice goal.

I'm not sure if there's one right approach to this. If you have checked reactabular you can see I favor very flexible design. With flexibility it's up to users to compose their own, specialized versions.

Let me know when you want some PR for your library and I'll see if I can arrange some. :+1: