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

Refactor of RYScaffold to use TopAppBar with scrolling behaviour #873

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yajaru
Copy link
Contributor

@yajaru yajaru commented Nov 13, 2024

This is a small refactor of the RYScaffold to use a TopAppBar with nested scrolling instead of an in-content header.

This also moves the Switch Account action from being on the Feeds page header text on-click to an actual dedicated button. I think this is more discoverable b/c it is not obvious how to switch accounts from the Feeds page if you don't already know about this feature.

Jump to top is also preserved for the browsing pages via an on-click callback on RYScaffold's new TopAppBar. This is basically what it was before, just move around a little.

Note that there is some goofyness around pages that are smaller that the viewport. Ideally I'd like them to not be scrollable at all but, I was not able to figure out how to do that elegantly. I could always pass down a boolean flag to RYScaffold to turn the scrolling off but, that feels nasty. If you have any ideas, let me know.

Before

before - unscrolled
before - scrolled

After

after - unscrolled
after - scrolled

@JunkFood02
Copy link
Collaborator

use a TopAppBar with nested scrolling instead of an in-content header

Why?

@JunkFood02
Copy link
Collaborator

The LargeTopAppBar component doesn't offer the flexibility we need. We can't customize the text styles for the large and small titles, the content padding, and other aspects. Additionally, using nestedScroll might lead to unexpected issues like what you've mentioned. I've implemented a workaround in #896. Please take a look and let me know if you're happy with the result

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.

2 participants