Skip to content

[core - v2.2.9] : Update - Device driven background scroll control #992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
May 16, 2022
Merged

[core - v2.2.9] : Update - Device driven background scroll control #992

merged 9 commits into from
May 16, 2022

Conversation

@Adamj1232
Copy link
Contributor

@Adamj1232 Adamj1232 commented May 9, 2022 •
edited
Loading

Description

A follow on the this PR and response to this issue
The goal here is to:

  • Prevent background scrolling while the onboard modal is open
  • Prevent vertical page jumps to the top of the page when the modal is opened

To accomplish this and avoid as many possible side affects as possible I when with targeting the HTML tag for controlling background positioning (mobile: position, desktop: overflow)
The reason for the different approaches is:
overflow = 'hidden' on mobile allows for a small amount of scrolling causing a gap at the bottom of the view port where the backdrop shadow doesn't cover
position = 'fixed' on desktop causes the page to jump to the top of the view port when the modal is opened

Checklist

  • The version field in package.json is incremented following semantic versioning
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works
  • This PR passes the Circle CI checks

@lnbc1QWFyb24
Copy link
Contributor

lnbc1QWFyb24 commented May 10, 2022

@Adamj1232 we also need to remove the scroll listener when the modal component is destroyed to prevent memory leaks.

@Adamj1232
Copy link
Contributor Author

Adamj1232 commented May 10, 2022

Yep totally agree, was refactoring that last night. I would be interested in your's and @taylorjdawson opinions on targeting the HTML tag for controlling background positioning (mobile: position, desktop: overflow)
The reason for the different approaches is:
overflow = 'hidden' on mobile allows for a small amount of scrolling causing a gap at the bottom of the view port where the backdrop shadow doesn't cover
position = 'fixed' on desktop causes the page to jump to the top of the view port when the modal is opened
The approach of targeting the HTML for setting these styles is that the HTML tag is typically not a target for a lot of base styles that would cause side-effects as you saw with targeting the body element @taylorjdawson
Open to ideas here

@taylorjdawson
Copy link
Contributor

taylorjdawson commented May 10, 2022

@Adamj1232 what's the goal of this PR exactly?

@taylorjdawson
Copy link
Contributor

taylorjdawson commented May 10, 2022

@Adamj1232 If I understand correctly you're saying to attach the modal as a child of the <html /> tag rather than the body?

@Adamj1232
Copy link
Contributor Author

Adamj1232 commented May 10, 2022

The goal is to control the background positioning while the Onboard modal is open - I updated the description to reflect more thoroughly

@Adamj1232 Adamj1232 changed the title [core - v2.2.9] : Update - Device driven background scroll [core - v2.2.9] : Update - Device driven background scroll control May 10, 2022
@Adamj1232 Adamj1232 changed the base branch from v2-web3-onboard to v2-web3-onboard-develop May 12, 2022 22:05
@ramsemune123 ramsemune123 mentioned this pull request May 13, 2022
Merged
4 tasks
@gesquinca
Copy link
Contributor

gesquinca commented May 13, 2022

Another option to consider:

disable scroll:
document.ontouchmove = function(e){ e.preventDefault(); }

enable scroll:
document.ontouchmove = function(e){ return true; }

I believe we could use scroll for desktop browsers and ontouchmove on mobile browsers.

@Adamj1232
Copy link
Contributor Author

Adamj1232 commented May 13, 2022

I was really hoping to find a fix like you have that works across browsers but wasn't able to find anything reliable.
My thinking for this was if we can scope the css changes<Mobile-position:fixed & Desktop-overflow:hidden> to the html tag we will avoid interfering with most styling approaches

@Adamj1232 Adamj1232 merged commit 509e69f into v2-web3-onboard-develop May 16, 2022
@Adamj1232 Adamj1232 deleted the fix/device_driven_background_scroll branch May 16, 2022 16:06
tokensex pushed a commit to tokensex/web3-onboard that referenced this pull request May 30, 2022
@Adamj1232 @tokensexdev
[core - v2.2.9] : Update - Device driven background scroll control (t…
910ce8d 
…hirdweb-dev#992)

* Refactor scroll handling to be device specific
* Refactor for cleanliness
* replace removeEventHandler and refactor
* Revert demo change
* Merge in new develop branch as base
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lnbc1QWFyb24 lnbc1QWFyb24 Awaiting requested review from lnbc1QWFyb24

1 more reviewer

@taylorjdawson taylorjdawson taylorjdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Adamj1232 @lnbc1QWFyb24 @taylorjdawson @gesquinca