search

naumch1k / one-fall

Landing page for ONE FALL, Salem-based melodic punk and hardcore quartet has rapidly made a mark in New England
https://develop--one-fall.netlify.app/
0 stars 0 forks source link

BUGFIX: layout shift and horizontal scrollbar on Windows OS #20

Closed Yura33-dev closed 3 months ago

Yura33-dev commented 3 months ago

Description

This PR removes a few problem which existing in project:

Task Link

This Pull request resolves #19

Changes Made

Some changes have been made in useModal and useOverlayMenu hooks as well as Navigation.module.css.

netlify[bot] commented 3 months ago

Deploy Preview for one-fall ready!

Name Link
Latest commit f3039bb4936e62166e596b6322d2a1ef32489bfc
Latest deploy log https://app.netlify.com/sites/one-fall/deploys/66a163d967ec640008954dab
Deploy Preview https://deploy-preview-20--one-fall.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Yura33-dev commented 3 months ago

Good morning, @naumch1k !

I have pushed a new changes according to your comment. But I don't sure about correctness of my code. Could you please check it ?) Thanks a lot!

Yura33-dev commented 3 months ago

Hi again, @naumch1k ! 👋

I have found a little bug with scroll at iOS devices. 🐞 But don't mind it, I have fixed it in my last commit. 😅 I have made some changes in getting of scrolled height approach. And got rid of smooth scrolling after close mobile menu. Now it works as expected! 🎉 But could you please recheck my code, when you have chance?

Also I have found way how to open website in local network at my mobile devices using WiFi. Thanks for your tips about it in today's meeting! 🤝

naumch1k commented 3 months ago

@Yura33-dev Glad to hear my tip was helpful!

Could briefly describe what was the bug you mentioned and what this code does please 🙏 This seems to run for all cases not only iOS devices, is that correct? 

      const getBodyTop: string = getComputedStyle(document.body).top
      const getScrollY: number = parseInt(getBodyTop.replace(/\D/g, ''), 10) || 0
Yura33-dev commented 3 months ago

Hey, @naumch1k !

There was a bug: when you open a website, then a little scroll it down (eg. 50px) this amount of px have been saved somewhere in memory and multiples by two each next reloaded the page. Thus, 50px changes to 100px, then 200px, then 400px and so on. And every page reload sends you lower and lower the page. To tell you the truth, I don't know why this bug happened.

This code: 

      const getBodyTop: string = getComputedStyle(document.body).top
      const getScrollY: number = parseInt(getBodyTop.replace(/\D/g, ''), 10) || 0

is more correctly way (especially for iOS) to get scrolled amount of px in order to return user at point where mobile menu has been opened.

  1. We get body's top property, which has been defined when mobile menu opened
  2. We remove all signs except numbers and change it to number type or just set 0 if something went wrong.

And yes, it works fine for all devices, not only for iOS.

naumch1k commented 3 months ago

@Yura33-dev Thanks for providing more details! I've asked my mentor, Oleksandr, to review this code since he was the first to identify the bug. He also experienced horizontal scrolling on Windows OS even before the first modal was opened. Hopefully, he'll find some time by the end of the week 😉

Yura33-dev commented 3 months ago

@Yura33-dev Thanks for providing more details! I've asked my mentor, Oleksandr, to review this code since he was the first to identify the bug. He also experienced horizontal scrolling on Windows OS even before the first modal was opened. Hopefully, he'll find some time by the end of the week 😉

So strange... I have not horizontal scrolling. Okay, just wait for his advice about it 🙂