Skip to content
This repository has been archived by the owner on Nov 27, 2022. It is now read-only.

fix: do not use animated scroll on initial render #1009

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

Conversation

jtomaszewski
Copy link
Contributor

Currently the component scrolls to the current tab with an animation, whenever:

  • layout dimensions change / are calculated
  • current route change
  • routes change

Which is good, but it causes the component to trigger a scroll animation on its' initial render, which might be undesired especially when you have multiple tabs, enabled scroll, and begin the view with not-the-first tab as the initial screen. The screen appears, the tab is highlighted as current... and the scroll animation starts.

Instead, I'd expect that on the initial render the screen already looks good (and the current tab is already positioned to the center), and only after that, for example when a route is changed, the view scrolls to the new current tab.

This PR makes it work in such a way.

Copy link
Owner

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

Instead, I'd expect that on the initial render the screen already looks good (and the current tab is already positioned to the center), and only after that, for example when a route is changed, the view scrolls to the new current tab.
This PR makes it work in such a way.

How can it work such a way? To have the correct scroll position on initial render, it would need to set something like contentOffset which is currently iOS only, there's a commit on master adding Android support but not in any release version.

And for auto width tabs, first we need to measure the tabs to know how much to scroll. Since measurements are not sync, it's impossible to ensure correct value on initial render.

When contentOffset support gets released for Android, it'll be possible to have correct position on initial render for non-auto-width tabs. But I don't think it's possible for initial render to already look as expected currently. If it did, then the animation won't make a difference anyway since it was already at the right place.

Removing animation would render the tab bar in wrong position initially, and then jump to correct position after calculations which is worse than animating to the correct position.

@jtomaszewski
Copy link
Contributor Author

jtomaszewski commented Jul 23, 2020

How can it work such a way? To have the correct scroll position on initial render, it would need to set something like contentOffset which is currently iOS only, there's a commit on master adding Android support but not in any release version.

In our case, we set width of each tab manually, and so it just works. (Because then both the screen width and tabs width are known prior to the component render.)

Removing animation would render the tab bar in wrong position initially, and then jump to correct position after calculations which is worse than animating to the correct position.

I guess it depends on your taste what's "worse"... We prefer having an instant render on a proper position than a needless animation that makes the user focus their attention on the tabs bar, when it's not really needed.

Nevertheless. If I made it configurable via a prop, would you merge this PR then?

@satya164
Copy link
Owner

I guess it depends on your taste what's "worse"... We prefer having an instant render on a proper position than a needless animation that makes the user focus their attention on the tabs bar, when it's not really needed.

It's not an instant render though, it's incorrect position initially and then jarring shifting to correct position, as opposed to a smooth animation.

Nevertheless. If I made it configurable via a prop, would you merge this PR then?

I think a prop is unnecessary, when we can handle it properly without needing an animation, we should handle it automatically: basically if width is already known (like you have), and there's support for contentOffset prop (currently on iOS, Android with next version of React Native), it'll be possible to have it in correct position.

So we should be able to set a contentOffset prop when we know the tab widths upfront to have it at correct position from the beginning. The animation related code doesn't need to be touched since if it's already at correct position, the animation won't do anything.

@satya164 satya164 changed the base branch from master to main October 7, 2020 08:23
@andreialecu
Copy link

As per facebook/react-native@ed29ba1 this should now land in RN 0.64 which is in RC and will be released soon.

Just a heads up, hopefully you can resume work on this PR as the functionality is sorely needed.

@Saad-Bashar
Copy link

Is it the reason that I can set any other tabs active on my initial render? Even after setting the index state to 1. @jtomaszewski

Here is the issue that I am facing btw - #1168

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

Successfully merging this pull request may close these issues.

None yet

4 participants