varunKT001 / tomper-wear-ecommerce

E-commerce web-application for selling clothing essentials 😀
https://tomper-wear.netlify.app
MIT License
38 stars 48 forks source link

Add ScrollToTop button component #40

Closed Nikhil-Sharma1 closed 2 years ago

Nikhil-Sharma1 commented 2 years ago

Issue reference: No Scroll To Top button in any page

Proposed changes: Add scroll to top button

Type of change: Adding scroll to top functionality

Please delete options that are not relevant.

Checklist:

Additional info (if any): I have added the Scroll-to-top button on every page. Since the height of the About page is less, so there is no need of scroll-to-top button there, hence the scroll-to-top button is not accessible there. If we include more data in that page and the height of that page is increasing in future, then the scroll-to-top button is appearing automatically after some scroll down. In other pages, it works fine. I have add transition effect on hover also. Please merge it with the main project.

I just add ScrollToTop folder in component folder to mantain the format and impoert that file file in app.js . So the format of the project does not change a bit.

Screenshot 1) Initially button is not visible Screenshot (302) 2) After some scroll down, button is visible Screenshot (307) 3) Transition effect on hover Screenshot (308)

netlify[bot] commented 2 years ago

👷 Deploy request for tomper-wear pending review. Visit the deploys page to approve it

🔨 Explore the source changes: 099967605c689ad21f1e90cac48a899771e69ee5

varunKT001 commented 2 years ago

Some improvements need

useEffect(() =>{
  window.addEventListener('scroll', toggleVisible);
  return () => window.removeEventListener('scroll', toggleVisible);
}, [])

Make these changes and tag me here 👍

Nikhil-Sharma1 commented 2 years ago

Thank you for giving me one more chance

On Fri, Mar 11, 2022, 10:50 Varun Tiwari @.***> wrote:

Some improvements need

  • You have made some changes in package.json. You changed the build scripts. Your change will make the build process fail. Remove all the changes done in the package.json.
  • The button doesn't look good. You had to use the color variables I made in the src/index.css.
  • You have created an event listener directly in the component. This is the worst way you can make event listeners. What will happen is, on every render, the component will get recreated. On every recreation of the component, new event listeners will get created. Therefore the thread will get loaded with tons of similar event listeners and the app will crash. For this you have to use useEffect()

useEffect(() =>{

window.addEventListener('scroll', toggleVisible);

return () => window.removeEventListener('scroll', toggleVisible); }, [])

Make these changes and tag me here 👍

— Reply to this email directly, view it on GitHub https://github.com/varunKT001/tomper-wear-ecommerce/pull/40#issuecomment-1064782218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQH3ZVZFBGIZVMUIPJREJWLU7LJ2XANCNFSM5QNFZ6LA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

varunKT001 commented 2 years ago

@Nikhil-Sharma1 It works fine, but you still need to make some changes:

Nikhil-Sharma1 commented 2 years ago

ok sir

On Sun, Mar 13, 2022, 12:31 Varun Tiwari @.***> wrote:

@Nikhil-Sharma1 https://github.com/Nikhil-Sharma1 It works fine, but you still need to make some changes:

  • Improve the UI.
    • Instead of a circle, use a square with some border-radius.
    • The icon is transparent so we can see things moving through it. Use some containers as a background.
    • Don't put any hover effect.
  • You should follow the code style. Remove the unnecessary blank lines that you have put. Only give one new line between each function.
  • If you are using vs-code, download the prettier extension, and from vs-code settings, enable format on save. If you are using some other editor, go to the prettier docs https://prettier.io/docs/en/editors.html to see how to integrate prettier with your editor. Make sure you format the code properly.

— Reply to this email directly, view it on GitHub https://github.com/varunKT001/tomper-wear-ecommerce/pull/40#issuecomment-1066040116, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQH3ZVZYC6P5FHNOQN5DJHLU7WHELANCNFSM5QNFZ6LA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

varunKT001 commented 2 years ago

@Nikhil-Sharma1 No need to call sir bro 😅 Also, if you need any help, just ping me on the discord server 👍

Nikhil-Sharma1 commented 2 years ago

ok bro😂

On Sun, Mar 13, 2022, 12:35 Varun Tiwari @.***> wrote:

@Nikhil-Sharma1 https://github.com/Nikhil-Sharma1 No need to call sir bro 😅 Also, if you need any help, just ping me on the discord server 👍

— Reply to this email directly, view it on GitHub https://github.com/varunKT001/tomper-wear-ecommerce/pull/40#issuecomment-1066040757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQH3ZV6QJHQRHMY6HFBCWKTU7WHT5ANCNFSM5QNFZ6LA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

Nikhil-Sharma1 commented 2 years ago

1)use transparent icon 2)change button shape Screenshot Screenshot (310) Screenshot (311)

varunKT001 commented 2 years ago

@Nikhil-Sharma1 First of all, really sorry for the delay, I was busy somewhere 😞

Now, you didn't do all the changes I asked for:

The icon is transparent so we can see things moving through it. Use some containers as a background.

I can still see the background moving from the button. Give the button background some primary color (you can try clr-primary-5). Also, make the icon color white. That will solve the problem.

Don't put any hover effect.

Remove the hover effect. It doesn't look good 😕

And finally the most important step, format all the files you change using prettier (code should not look ugly 🙂)

Do these changes and tag me here 👍

Nikhil-Sharma1 commented 1 year ago
Sir, I have done the required changes. So please close my previous pull request and I have generated new pull request Sent from Mail for Windows From: Varun TiwariSent: Friday, March 11, 2022 10:50 AMTo: varunKT001/tomper-wear-ecommerceCc: Nikhil Sharma; AuthorSubject: Re: [varunKT001/tomper-wear-ecommerce] Add ScrollToTop button component (PR #40) Some improvements needYou have made some changes in package.json. You changed the build scripts. Your change will make the build process fail. Remove all the changes done in the package.json.The button doesn't look good. You had to use the color variables I made in the src/index.css.You have created an event listener directly in the component. This is the worst way you can make event listeners. What will happen is, on every render, the component will get recreated. On every recreation of the component, new event listeners will get created. Therefore the thread will get loaded with tons of similar event listeners and the app will crash. For this you have to use useEffect()useEffect(() =>{   window.addEventListener('scroll', toggleVisible);   return () => window.removeEventListener('scroll', toggleVisible); }, [])Make these changes and tag me here 👍—Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>