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

fix: support scroll to top on profile screen #725

Merged
merged 6 commits into from
Jun 1, 2023

Conversation

kadikraman
Copy link
Contributor

Fixes #719

This follows the existing pattern used on the other tabs: we listen to the onScreenSoftReset event and scroll to top when it is fired.

The diff looks bigger than the actual change (due to the forwardRef indentation), the main thing was wrapping ViewSelector in a forwardRef and adding this:

React.useImperativeHandle(ref, () => ({
  scrollToTop: () => {
    flatListRef.current?.scrollToOffset({offset: 0})
  },
}))

so it could be called from the ProfileScreen when a soft reset is triggered.

Before

profile-scroll-before.mp4

After

profile-scroll-after.mp4

@renahlee renahlee requested a review from pfrazee May 18, 2023 16:16
@piotrski
Copy link

Instead of using useImperativeHandle to handle the scroll-to-top behavior, I suggest using the useScrollToTop hook from react-navigation. It's a more convenient option provided by the library. Check out the documentation for more details.

@kadikraman
Copy link
Contributor Author

@piotrski for sure, I thought the same, but it is not used for the other tabs (I think because they want to do other actions like refresh the feed as well as scroll to top), so it seemed better to use an existing pattern instead of introducing something new for one tab. For reference, here's Search, Notifications, Home.

@ansh
Copy link
Contributor

ansh commented May 31, 2023

Thank you for this! It would be better to use the existing button we have for scrolling to top. Just so we can match the current style on every page.

@ansh ansh added the x:discussing We've seen the request and we're talking about it! label May 31, 2023
@kadikraman
Copy link
Contributor Author

Hi @ansh would you mind explaining what you mean by "existing button"? This uses the same pattern as the other screens. The only difference is that the scrollable page is in a component so we're having to pass the scroll to top event one level down.

@pfrazee
Copy link
Collaborator

pfrazee commented Jun 1, 2023

@kadikraman I think ansh was referring to the useMainScroll and "scroll to top button" facilities we recently ironed out

https://github.com/bluesky-social/social-app/blob/main/src/view/screens/CustomFeed.tsx#L55

https://github.com/bluesky-social/social-app/blob/main/src/view/screens/CustomFeed.tsx#L346

However, I'm pretty sure the changes you've made here are independent of that and a necessary precursor so I'll give this a look. (This is one of those diffs that looks way worse than it is due to the indentation change.)

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Yeah looks great. Well done, thank you!

@pfrazee pfrazee merged commit d4e7355 into bluesky-social:main Jun 1, 2023
@kadikraman kadikraman deleted the fix/profile-scroll-to-top branch June 1, 2023 16:05
alimony added a commit to alimony/social-app that referenced this pull request Jun 8, 2023
* main: (27 commits)
  Revert "show date after 7 days closes bluesky-social#754" (bluesky-social#860)
  push instead of navigate (bluesky-social#861)
  [APP-680] Allow users to add details when reporting (bluesky-social#854)
  allow image to clicked to go to post in notificaitons (bluesky-social#858)
  Use expo-image-picker on Web (bluesky-social#847)
  remove X-UA-Compatible meta tag (bluesky-social#857)
  remove trailing / on meta tags (bluesky-social#856)
  [APP-655] Password autocomplete when logging in (bluesky-social#838)
  Fix dark mode detection on mobile (bluesky-social#852)
  1.30
  Fix to simulator tests
  [APP-107] OTA updates (bluesky-social#587)
  Move suggested follow recommendations to the server (bluesky-social#836)
  Fix a bunch of type errors and add a type-check to the github workflows (bluesky-social#837)
  Don't show Remove button for nonexistent avatar/banner (bluesky-social#833)
  More custom-feed behavior fixes [APP-678] (bluesky-social#831)
  Fixes to feed preference and state sync [APP-678] (bluesky-social#829)
  bskyweb: add security.txt
  fix: support scroll to top on profile screen (bluesky-social#725)
  Fix line breaks on side bar links (bluesky-social#815)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:discussing We've seen the request and we're talking about it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tapping profile tab/icon while active should scroll to the top
4 participants