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

Find wallet > remove all the scrolls > quick fix #11174

Merged
merged 29 commits into from
Oct 2, 2023

Conversation

nloureiro
Copy link
Contributor

@nloureiro nloureiro commented Sep 18, 2023

The find wallet page was designed to have a specific behavior that evolved to add some scroll in some areas of the page.
we got some feedback that was making the experience worse, and we wanted to test without the scroll.

This PR is trying to test and probably implement a simpler experience on this page.

Preview link

https://ethereumorgwebsitedev01-findwalletremovescroling.gatsbyjs.io/en/wallets/find-wallet

Related Issue

#11156

@nloureiro nloureiro self-assigned this Sep 18, 2023
@nloureiro nloureiro linked an issue Sep 18, 2023 that may be closed by this pull request
2 tasks
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 18, 2023

✅ ethereum-org-website-dev deploy preview ready

@minimalsm minimalsm changed the title Find walltet > remove all the scrolls > quick fix Find wallet > remove all the scrolls > quick fix Sep 18, 2023
@wackerow
Copy link
Member

Awesome! I definitely think this feels better...

image

I suppose my main concern is if we should make anything sticky... such as potentially the left filter menus, or more importantly the top filter bar.

I think I care most about the top filter bar since if you lose that, the green checks mean nothing. Those column headers are dynamic so without those, you really have no idea what the checks/x's mean.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Good improvement!

Agreed with @wackerow, would be a good addition to have the filters and table header as sticky to keep improving the ux. I think you mentioned this on the original issue and that could come in next iterations.

=====

Two things I see while testing:

  1. In mobile, the filter's sidebar doesn't work very well when scrolling too much
    https://github.com/ethereum/ethereum-org-website/assets/468158/b79bfaec-439e-42e4-ac5c-7a7e3b9871b4

  2. Can we remove the bottom 1px line that separate the table's content from the footer? made sense before since we had that inner scrolling but now it feels unnecessary and we end having 2 close bottom lines
    image

src/pages/wallets/find-wallet.tsx Outdated Show resolved Hide resolved
src/pages/wallets/find-wallet.tsx Outdated Show resolved Hide resolved
src/pages/wallets/find-wallet.tsx Outdated Show resolved Hide resolved
@wackerow
Copy link
Member

Added some changes... to summarize:

  • Removed some of the logic from the WalletFilterSidebar that was handling mobile vs desktop styling (made it more "dumb")
  • Implemented a Chakra-UI Drawer component to handle the entire mobile display for the WalletFilterSidebar component, including open/close logic (implemented with useDisclosure Chakra-UI hook), animation, and handling of the backdrop, focus trapping, and pointer events.
  • Removed the boxShadow being used as a backdrop
  • Removed all pointerEvents overrides from these components (the Drawer should handle everything that we needed these for)

wackerow and others added 4 commits September 28, 2023 16:26
docs/ds-implementation.md Outdated Show resolved Hide resolved
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Just tried this out, feeling much more polished! Got all the sticky positioning cleaned up. Approved from my end.

@corwintines
Copy link
Member

corwintines commented Oct 2, 2023

Screen.Recording.2023-10-01.at.8.13.44.PM.mov

Added a line for full width back in to avoid this resizing bug on desktop

update to Chakra-UI Text component for clean up
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Looks great!

Wondering if we consider the following as a bug:

issue.mp4

I'd expect the content of the sidebar to be sticky as well.

Approving it for now.

@corwintines corwintines merged commit 9cf58ca into dev Oct 2, 2023
4 checks passed
@corwintines corwintines deleted the find-wallet-remove-scroling branch October 2, 2023 15:46
This was referenced Oct 4, 2023
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.

remove scroll bar on find-wallet page
4 participants