RocketChat / RC4Community

Full-stack components for building, engaging, and growing your massive on-line community
https://community.rocket.chat/
Apache License 2.0
48 stars 68 forks source link

Fixes carousel bug #199

Closed vasucp1207 closed 1 year ago

vasucp1207 commented 1 year ago

https://user-images.githubusercontent.com/85363195/201475252-0699b0cb-cfbc-4354-9f06-1e527c611b10.mov

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

Palanikannan1437 commented 1 year ago

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 soft resetting to the commit before that and then force push your changes again such that your package-lock.json isn't there in your git diff.

Thank you 😄

vasucp1207 commented 1 year ago

@Palanikannan1437, I do not change anything in package.json, I don't know how it changes automatically. I commit the changes but it appears again can you help me?

Palanikannan1437 commented 1 year ago

@Palanikannan1437, I do not change anything in package.json, I don't know how it changes automatically. I commit the changes but it appears again can you help me?

Hey yeah sure, so when you install a package using the npm install .... command or if it's internally run due to our existing scripts, the package.lock.json is generated/modified

So basically while staging your files before committing, instead of git add . you'd have to manually stage the files you changed and want to commit.😄

vasucp1207 commented 1 year ago

@Palanikannan1437 I remove the package-lock.json is it okay now please tell me.

Palanikannan1437 commented 1 year ago

@Palanikannan1437 I remove the package-lock.json is it okay now please tell me.

Sorry, but still it's in the git diff

image

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 soft resetting to the commit before that and then force push your changes again such that your package-lock.json isn't there in your git diff.

Thank you 😄

Please try to research and follow up on this approach of soft resetting and force pushing in git and let us know if you face any issues. Thank you!

vasucp1207 commented 1 year ago

@Palanikannan1437 I think it's done now.

Palanikannan1437 commented 1 year ago

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 onClick method on our custom PrevArrow and NextArrow by going through react-slick's custom arrow documentation.

Thank you and hope to see your PR get merged soon 😄

Palanikannan1437 commented 1 year ago

@Palanikannan1437 I think it's done now.

That was awesome and kudos on being able to remove the package-lock.json from your PR!!

Hope you got to learn something out of it!!

vasucp1207 commented 1 year ago

@Palanikannan1437, but what is the need to add the custom arrows when react-slick provides the arrows by default?

Palanikannan1437 commented 1 year ago

@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!

Sing-Li commented 1 year ago

@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.

vasucp1207 commented 1 year ago

@Sing-Li, fixed.

Palanikannan1437 commented 1 year ago

LGTM @vasucp1207 !! 🥳

@Sing-Li I've reviewed the changes, please consider merging them, thank you!

Sing-Li commented 1 year ago

Congrats @vasucp1207