Fixes carousel bug #199
Fixes carousel bug #199
Conversation
|
|
There was a problem hiding this comment.
Hey @vasucp1207, it's not a good idea to commit package-lock.json, it'll cause problems for others most of the time. Please make the necessary changes to your PR
Thank you 😊
|
Hey @vasucp1207, sorry for not being very clear...you didn't have to delete the package-lock.json file, you just have to remove it from your PR. One approach could be...you could revert your old commit's changes by Thank you 😄 |
|
@Palanikannan1437, I do not change anything in package.json, I don't know how it changes automatically. |
Hey yeah sure, so when you install a package using the So basically while staging your files before committing, instead of |
|
@Palanikannan1437 I remove the package-lock.json is it okay now please tell me. |
Sorry, but still it's in the git diff
Please try to research and follow up on this approach of |
|
@Palanikannan1437 I think it's done now. |
|
Hey @vasucp1207, that was a great catch and thank you for your valuable contribution! But currently there's no need to change the existing custom button ui/button component on our slider...instead it'd be better if you could debug and add the missing Thank you and hope to see your PR get merged soon 😄 |
That was awesome and kudos on being able to remove the Hope you got to learn something out of it!! |
|
@Palanikannan1437, but what is the need to add the custom arrows when react-slick provides the arrows by default? |
Hey, I'm not sure but it was a design choice made by our contributors...and since there's just a small function they missed, there's no need to remove the entire existing button components but instead you could add the missing function. @Sing-Li ,@Dnouv and @irffanasiff could better guide here, thank you! |
|
@vasucp1207 Yes. As @Palanikannan1437 mentioned --> the custom arrows are BY DESIGN. We had a designer contributor making sure that the UI elements are consistent, color schemes are attractive etc. This is typical of all production project. You can't use the default "just because it is there". Please update your PR asap. |
|
@Sing-Li, fixed. |
app/components/newscarousel.js
Outdated
| @@ -23,6 +25,7 @@ const PrevArrow = ({ currentSlide, slideCount, ...props }) => { | |||
| }; | |||
|
|
|||
| const NextArrow = ({ currentSlide, slideCount, ...props }) => { | |||
There was a problem hiding this comment.
You could simply de-structure the onClick method here itself! Thank you!
app/components/newscarousel.js
Outdated
| @@ -5,6 +5,7 @@ import 'slick-carousel/slick/slick-theme.css'; | |||
| import Image from "next/image"; | |||
|
|
|||
| const PrevArrow = ({ currentSlide, slideCount, ...props }) => { | |||
There was a problem hiding this comment.
Likewise, you could de-structure the onClick method here itself! Thank you!
|
LGTM @vasucp1207 !! 🥳 @Sing-Li I've reviewed the changes, please consider merging them, thank you! |
|
Congrats @vasucp1207 |
Screen.Recording.2022-11-12.at.6.31.42.PM.mov