Skip to content
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.

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

3020: Fix unexpected scrolling behaviour in mobile desktop #3050

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lunars97
Copy link
Contributor

Short description

Currently the category pages are having additional scrollable blank height after the toolbar footer on the small screens if the side kebab-menu is activated. This might be happening because of the visibility: hidden attribute of the Portal potentially taking up unintended space due to it being a part of the DOM, therefore it could still occupy space.

Proposed changes

  • use display instead of visibility
  • add relative position to the parent element (body), because of the child Portal element having the absolute position
  • add z-index to be placed above the category elements
  • refactor window.scrollY to calculate and memoize the value and to make sure the side menu starts at the top of the screen

Testing

  • Open the side-menu on the small screen and check the category and its children to see if there is additional height after the toolbar footer and try to open the side-menu as well as inside category children

Side effects

-should be none?

Resolved issues

Fixes: #3020


@lunars97 lunars97 force-pushed the 3020-fix-unexpected-scrolling-behaviour-in-mobile-desktop branch from feb9bb7 to 38729d8 Compare January 26, 2025 18:57
Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunars97 are all of these changes necessary to fix the issue? Can you elaborate which change fixes which problem?

web/src/App.tsx Show resolved Hide resolved
@lunars97
Copy link
Contributor Author

@lunars97 are all of these changes necessary to fix the issue? Can you elaborate which change fixes which problem?

yes, these changes should fix the issue

  • using display instead of visibility fixes the unexpected scroll behaviour under the footer, because visibility:hidden when the side-menu is hidden visually, it stays as a part of DOM which might occupy the space after using the side-menu and lead to unexpected extra space after hiding the side-menu.
  • adding relative position to the parent element (body), because of the child Portal element having the absolute position which can fix the side-menu appearing in the middle of the view
  • adding z-index to be placed above the category elements makes sure that it will appear above the category view not under
  • by refactoring window.scrollY to calculate and memoize the value and to make sure the side menu starts at the top of the screen also helps with the problem of side-menu appearing in the middle of the screen

@lunars97 lunars97 force-pushed the 3020-fix-unexpected-scrolling-behaviour-in-mobile-desktop branch from 38729d8 to 1f118a5 Compare January 27, 2025 13:17
Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix, thank you!

web/src/App.tsx Outdated Show resolved Hide resolved
@lunars97 lunars97 force-pushed the 3020-fix-unexpected-scrolling-behaviour-in-mobile-desktop branch from 1f118a5 to 0d501c2 Compare January 27, 2025 20:38
@lunars97 lunars97 force-pushed the 3020-fix-unexpected-scrolling-behaviour-in-mobile-desktop branch from 0d501c2 to 871aa75 Compare January 27, 2025 20:48
Copy link
Contributor

@bahaaTuffaha bahaaTuffaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution 🤩 from now on I will keep an eye on any visibility propriety that could cause issues like this ... that was unexpected.

visibility: show ? 'visible' : 'hidden',
top: window.scrollY > 0 ? `${window.scrollY}px` : undefined,
display: show ? 'block' : 'none',
top: scrollY > 0 ? `${scrollY}px` : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 I tried to remove this top completely and it didn't affect anything at this page not sure if we can remove it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair question,I think we do not need it here completely

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1sh1918 do you still remember why we needed this in the first place? To me it also looks like it works just fine without setting anything as top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected scrolling behavior in desktop mobile
3 participants