Feature/tech 438 hero component #7
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@ikhoaA thanks for this PR! Could you please resolve the conflict with |
There was a problem hiding this comment.
@ikhoaA sorry for the wait on this one. This required a little bit of my time to make sure I could provide constructive feedback. Overall it's great work. However, there's a few items for you to review. This is an open discussion, so feel free to let me know if you disagree on some points or if you have alternative solutions to offer. Thanks!
There was a problem hiding this comment.
Thanks for implementing the previous change request @ikhoaA. This is back to you for another round.
In your next contributions, please try to be particularly mindful of package size when you add a dependency. Any dependency added to the project bloats the node_modules directory and impacts performance, so I'd like to see the rationale behind the addition when you deem it necessary.
Thanks and hopefully this should be the last change request on this one!
package.json
Outdated
| "next": "^12.1.0", | ||
| "prop-types": "^15.8.1", | ||
| "react": "^16.8.0 || ^17", | ||
| "react-dom": "^16.8.0 || ^17", | ||
| "react-easy-swipe": "^0.0.22", |
There was a problem hiding this comment.
What's the rationale for adding react-easy-swipe? Does it do the same thing as react-swipeable? Since it's almost 200KB, I'd rather avoid the extra weight on the node_modules directory, if possible.
lib/Hero/Carousel.js
Outdated
| import { AiOutlineLeft, AiOutlineRight } from "react-icons/ai"; | ||
| import Swipe from "react-easy-swipe"; | ||
|
|
||
| const Carousel = ({children}) => { |
There was a problem hiding this comment.
Leave spaces in your object destructuring notation: const Carousel = ({ children }) => {
lib/Hero/Carousel.js
Outdated
| ) | ||
| } | ||
| <div> | ||
| </div> |
lib/Hero/Hero.js
Outdated
| ); | ||
|
|
||
|
|
There was a problem hiding this comment.
No need for an extra line break here
lib/Hero/Hero.js
Outdated
| title: PropTypes.string.isRequired, | ||
| /** | ||
| * Subtitle of Hero component | ||
| * e.g. `How to make you feel good` |
There was a problem hiding this comment.
I appreciate the effort, but I don't think we necessarily need these examples. The names of the props are self-explanatory and the types provide enough info as to what they should be.
lib/Hero/Hero.js
Outdated
| * Hero image URL | ||
| * e.g. `https://images.unsplash.com/photo-1469334031218-e382a71b716b?ixlib=rb-1.2.1&ixid=MnwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8&auto=format&fit=crop&w=2670&q=80` | ||
| */ | ||
| imageUrl: PropTypes.string , |
There was a problem hiding this comment.
Let's remove the space before this trailing coma
stories/Hero.stories.jsx
Outdated
| }; | ||
| NextImage.args = { | ||
| label: 'Hero', | ||
| Button, |
There was a problem hiding this comment.
Let's fix the indentation here. I'd recommend running Prettier on this branch.
lib/Hero/Carousel.js
Outdated
| import { AiOutlineLeft, AiOutlineRight } from "react-icons/ai"; | ||
| import Swipe from "react-easy-swipe"; | ||
| import React, { useState } from 'react'; | ||
| import leftArrow from '../../public/arrow left.svg'; |
There was a problem hiding this comment.
I would highly discourage using spaces in file names. That's a big no-no in my books.
lib/Hero/Carousel.js
Outdated
| return ( | ||
| <div className="m-auto"> | ||
| <div className="w-full relative select-none"> | ||
| <AiOutlineLeft | ||
| <img |
There was a problem hiding this comment.
Let's avoid using an img tag for an SVG when we can render the SVG tags directly in the DOM instead. We've done it for LeftChevron and RightChevron in the ImageGallery component. And now that I think about it, it looks like we can re-use these same SVG paths and use a different fill color.
In this case, let's move lib/ImageGallery/LeftChevron.js and lib/ImageGallery/RightChevron.js to lib/Icons/, change the imports in lib/ImageGallery/ImageGallery.js and use lib/Icons/* in lib/Hero/Carousel.js.
lib/Hero/Carousel.js
Outdated
| ); | ||
| })} | ||
| </div> | ||
| <AiOutlineRight | ||
| <img |
There was a problem hiding this comment.
Same comment as for the left arrow.
No description provided.