Remove vertical scrollbars from views on native #3429
Conversation
|
| @@ -130,6 +131,7 @@ export function RecommendedFeeds({next}: Props) { | |||
| renderItem={({item}) => <RecommendedFeedsItem item={item} />} | |||
| keyExtractor={item => item.uri} | |||
| style={{flex: 1}} | |||
| showsVerticalScrollIndicator={false} | |||
There was a problem hiding this comment.
These screens actually look better in general without indicators (and already do not show them on web) so let's just set the prop to false.
haileyok
force-pushed
the
hailey/remove-vertical-scroll-bar-native
branch
from
f14985c to
9d6053a
Compare
src/view/com/util/List.tsx
Outdated
| const showsVerticalScrollIndicator = | ||
| useGate('shows_vertical_scroll_indicator') && isWeb |
There was a problem hiding this comment.
I am using shows_vertical_scroll_indicator for the gate name, even though we're actually removing the indicators. I think it makes more sense to use the wording shows_vertical_scroll_indicator just so we are not inverting the boolean. We can change the name though if we want to keep the gate name identical to what we are actually testing.
There was a problem hiding this comment.
Ah, I just realized this isn't ideal because the useGate will return false IIRC. Just going to use hide_vertical_scroll_indicators instead.
src/view/com/util/Views.jsx
Outdated
| const showsVerticalScrollIndicator = useGate( | ||
| 'shows_vertical_scroll_indicator', | ||
| ) |
There was a problem hiding this comment.
This component is only used on native, so we don't need an isWeb check here.
src/view/com/util/List.tsx
Outdated
| @@ -37,7 +40,8 @@ function ListImpl<ItemT>( | |||
| const isScrolledDown = useSharedValue(false) | |||
| const contextScrollHandlers = useScrollHandlers() | |||
| const pal = usePalette('default') | |||
|
|
|||
| const showsVerticalScrollIndicator = | |||
| !useGate('hide_vertical_scroll_indicators') && isWeb | |||
There was a problem hiding this comment.
should this be && or should it be || ?
There was a problem hiding this comment.
God I had my explanation typed out and then realized you're right. What a mind fuck. Sec.
There was a problem hiding this comment.
okay fixed...
Case 1:
- Gate false -> inverted -> true
- Is not on web -> false
- Shows indicator
Case 2:
- Gate false -> inverted -> true
- Is on web -> true
- Shows indicator
Case 3:
- Gate true -> inverted -> false
- Is not on web -> false
- Doesn't show
Case 4:
- Gate true -> inverted -> false
- Is on web -> true
- Shows indicator
Rationale
Because a lot of our lists are virtualized, there is a bit of "jank" whenever the view resizes. For example in the feed, each time a new batch of items gets rendered below our current scroll position, the scrollbar changes position. Additionally, whenever we load the next batch of items from the server, the scrollbar jumps up the page to account for the new items.
Other apps, such as Meta products, do not show the vertical scrollbar in any scroll view - including non-virtual lists such as settings, etc.
My opinion is that we definitely should now show scrollbars on our virtualized lists for two reasons:
And since we are removing the scrollbars from the main parts of the app, I also believe we should just remove them from the other few
ScrollViewareas. I have two reasons for that as well:react-navigation, primarily.How
This was pretty simple. There are two places where we get these:
ListandScrollView. Thankfully, we have nice little wrappers for these already, so I only needed to modify this prop in two places. I went through the app and can't find anywhere that I missed this.There were a few places that we are using
FlatListdirectly as well, which can get fixed in a separate PR (there's some other things there that are no good like using anonymous functions as props). For now I addedshowsVerticalScrollIndicator={false}to thoseFlatLists manually.Test Plan
This is mainly a visual change, so really its a matter of going around the app and seeing if not having the scrollbars is something that seriously bothers you or not. We might decide some areas need them, but from my experience so far I have not ran into any of those cases.