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

[ABW-3015] AccountDetails - Collapse header on scroll #1068

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

danvleju-rdx
Copy link
Contributor

@danvleju-rdx danvleju-rdx commented Apr 2, 2024

Jira ticket: ABW-3015

Description

Adds a generic component that creates a collapsible header with a list, used in the Account Details screen.

Demo

Screen.Recording.2024-04-02.at.12.59.48.mov

@danvleju-rdx danvleju-rdx added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Apr 2, 2024
@CyonAlexRDX
Copy link
Contributor

should we really hide the bar "Tokens" / "NFT" / "Staking" / "Pool" here? I would I expect it to be sticky, but maybe Matt did not want that?

Screenshot 2024-04-02 at 15 03 06

@danvleju-rdx
Copy link
Contributor Author

should we really hide the bar "Tokens" / "NFT" / "Staking" / "Pool" here? I would I expect it to be sticky, but maybe Matt did not want that?

Screenshot 2024-04-02 at 15 03 06

Hmm, I don't think there was any requirement regarding this. I was guided by how it is done on Android and the fact that it is not sticky on the current implementation. But it can be done easily.

Copy link
Contributor

@kugel3 kugel3 left a comment

Choose a reason for hiding this comment

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

This looks very nice, but we do have as a goal to remove all usage of Introspect. Could this not be done instead with an GeometryReader/Preference combo until we can require iOS 17? We already have some helpers defined for that.

If you place it on the upmost view in the scrollview, you will be able to get the content offset as long the it is (more or less) visible, but we also know that when it disappears, the header should also be collapsed.

Another option we should consider is to use a UITableView here, we do that in AccountHistory, mostly for performance reasons, but it also makes things like this easier.

@danvleju-rdx
Copy link
Contributor Author

@kugel3 Indeed, it will be easier and look better to use a UITableView. Ghenadie mentioned that we do have a goal to use a UITableView instead of a List here, but it will take more time, and it's not a priority for now, so this is more like a temporary solution until we do the refactoring.

If I place the header view in the List, then I should wrap the List's views inside a ZStack (as the header should be under the content), and it will break the style.

@GhenadieVP
Copy link
Contributor

Looks slick, though there are some rough edges, which I am not sure easy are fixable. Here I have few resources in my account, so the scroll content is reduced.
Additionally, some deceleration would be nice when user stops scrolling, but probably I am getting to fancy :)

RPReplay_Final1712243903.MP4

@danvleju-rdx
Copy link
Contributor Author

danvleju-rdx commented Apr 4, 2024

@GhenadieVP Reasonably fancy :). I did some improvements here. Feels much better.

There's a constant called velocityThreshold that can be modified to adjust scroll sensitivity. I set it to 900 as this value seems more appropriate.

RPReplay_Final1712253758.MP4

Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

Awesome!
Note - this is a temporary solution to make it work with SwiftUI.
We will have a better, simpler implementation when we will get to have the whole AssetsView rewritten with UIKit.

@GhenadieVP GhenadieVP merged commit 25b9d34 into main Apr 9, 2024
6 checks passed
@GhenadieVP GhenadieVP deleted the ABW-3015-account-details-collapse-header-on-scroll branch April 9, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs
Development

Successfully merging this pull request may close these issues.

5 participants