PedroBern / react-native-collapsible-tab-view

A cross-platform Collapsible Tab View component for React Native
MIT License
836 stars 162 forks source link

Feat react navigation #58

Closed PedroBern closed 3 years ago

PedroBern commented 3 years ago

react-navigation support on v3 🎉

I will update the readme and improve the examples before merging

@andreialecu

// example usage

import { createCollapsibleNavigator } from 'react-native-collapsible-tab-view'

export type Params = {
  article: undefined
  albums: undefined
  contacts: undefined
}

const Tabs = createCollapsibleNavigator<Params>()

// you get:
// Tabs.Navigator
// Tabs.Screen
// Tabs.FlatList
// Tabs.ScrollView

<Tabs.Navigator containerRef={containerRef} refMap={refMap} {...props}>
    <Tabs.Screen
      name="article"
      component={ArticleScreen}
      options={{ tabBarLabel: 'Article' }}
    />
    <Tabs.Screen
      name="albums"
      component={AlbumsScreen}
      options={{ tabBarLabel: 'Albums' }}
    />
    <Tabs.Screen
      name="contacts"
      component={ContactsScreen}
      options={{ tabBarLabel: 'Contacts' }}
    />
</Tabs.Navigator>

Closes #64 Closes #71

github-actions[bot] commented 3 years ago

The Expo app for the example from this branch is ready!

expo.io/@pedrobern/react-native-collapsible-tab-view-demos?release-channel=pr-58

andreialecu commented 3 years ago

Hey, so I wanted to try this but it seems that when adding it as a git dependency, it fails to build:

src/Navigator/createCollapsibleNavigator.tsx:34:10 - error TS2742: The inferred type of 'createCollapsibleNavigator' cannot be named without a reference to 'react-native/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.

34 function createCollapsibleNavigator<T extends ParamListBase>() {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~

src/Navigator/createCollapsibleNavigator.tsx:34:10 - error TS4058: Return type of exported function has or is using name 'AnimatedNode' from external module "react-native-reanimated" but cannot be named.

34 function createCollapsibleNavigator<T extends ParamListBase>() {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~

Found 2 errors.

The version I was using previously worked via its prepare script, something probably regressed in that area.

    "react-native-collapsible-tab-view": "PedroBern/react-native-collapsible-tab-view#feat-react-navigation",
andreialecu commented 3 years ago

As a quick repro (I'm using yarn 2):

Make sure you have a recent yarn 1 version:

npm install -g yarn
cd `mktemp -d` # switch to a temp dir
yarn init -2 # init on yarn v2
yarn config set nodeLinker node-modules
yarn config set enableInlineBuilds true # makes error easier to see

yarn add react-native-collapsible-tab-view@PedroBern/react-native-collapsible-tab-view#feat-react-navigation
andreialecu commented 3 years ago

Easier repro:

I cloned the branch this PR is based on. Running yarn prepack results in the errors above.

PedroBern commented 3 years ago

I found this on StackOverflow, same error with a different package, maybe the second answer help

https://stackoverflow.com/questions/63806168/need-help-fixing-or-suppressing-this-tslint-error-ts2742

I will try to reproduce here

PedroBern commented 3 years ago

Same error here 😞 I will investigate

PedroBern commented 3 years ago

try now, should work

andreialecu commented 3 years ago

You can fix one of the errors by removing @types/react from devDependencies which shouldn't be needed I think. @types/react-native has react types already. In my RN projects I don't add @types/react.

The problem with the latest commit is that it loses all types, so code completion won't work

PedroBern commented 3 years ago

You can fix one of the errors by removing @types/react from devDependencies which shouldn't be needed I think. @types/react-native has react types already. In my RN projects I don't add @types/react.

Thanks for the suggestion, I will do it

The problem with the latest commit is that it loses all types, so code completion won't work

Yes I know, I will fix it. Was just a quick workaround.

andreialecu commented 3 years ago

The other error seems to originate from the ScrollView export, aka these two:

// Tabs.FlatList
// Tabs.ScrollView

I haven't looked too deeply into the changes, but why are they reexported from here?

andreialecu commented 3 years ago

To clarify, this diff resolves the build error:

diff --git a/package.json b/package.json
index ee3aef7..b40c5d1 100644
--- a/package.json
+++ b/package.json
@@ -48,7 +48,6 @@
     "@react-native-community/bob": "^0.16.2",
     "@react-navigation/native": "^5.9.2",
     "@release-it/conventional-changelog": "^1.1.4",
-    "@types/react": "^16.9.44",
     "@types/react-native": "0.63.4",
     "babel-jest": "^26.2.2",
     "babel-preset-react-native": "^4.0.0",
diff --git a/src/Navigator/createCollapsibleNavigator.tsx b/src/Navigator/createCollapsibleNavigator.tsx
index d784e83..4106944 100644
--- a/src/Navigator/createCollapsibleNavigator.tsx
+++ b/src/Navigator/createCollapsibleNavigator.tsx
@@ -32,12 +32,7 @@ import {
 } from './types'

 function createCollapsibleNavigator<T extends ParamListBase>() {
-  const {
-    FlatList,
-    ScrollView,
-    Container,
-    useTabsContext,
-  } = createCollapsibleTabs<keyof T>()
+  const { Container, useTabsContext } = createCollapsibleTabs<keyof T>()

   const Context = React.createContext<TabOptionsContext<keyof T> | undefined>(
     undefined
@@ -234,8 +229,6 @@ function createCollapsibleNavigator<T extends ParamListBase>() {
   return {
     Navigator,
     Screen,
-    FlatList,
-    ScrollView,
     useTabsContext,
     useTabOptionsContext,
   }
diff --git a/yarn.lock b/yarn.lock
index d6a29fa..3399904 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -2058,14 +2058,6 @@
     "@types/prop-types" "*"
     csstype "^3.0.2"

-"@types/react@^16.9.44":
-  version "16.14.2"
-  resolved "https://registry.yarnpkg.com/@types/react/-/react-16.14.2.tgz#85dcc0947d0645349923c04ccef6018a1ab7538c"
-  integrity sha512-BzzcAlyDxXl2nANlabtT4thtvbbnhee8hMmH/CcJrISDBVcJS1iOsP1f0OAgSdGE0MsY9tqcrb9YoZcOFv9dbQ==
-  dependencies:
-    "@types/prop-types" "*"
-    csstype "^3.0.2"
-
 "@types/responselike@*", "@types/responselike@^1.0.0":
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/@types/responselike/-/responselike-1.0.0.tgz#251f4fe7d154d2bad125abe1b429b23afd262e29"

But not sure if removing those is correct.

PedroBern commented 3 years ago

Yes, I just saw it too, the error comes from scroll view. We need to re-export because they are "connected" to the onScroll of each tab. The Tabs.ScrollView knows if the parent component/tab is focused or not, allowing the Tabs.Container to track the correct y position (from the currently focused tab) and know how to calculate the offset.

PedroBern commented 3 years ago

now it's fixed 👍

- export type ScrollViewProps = ComponentProps<typeof Animated.ScrollView>
+ export type ScrollViewProps = ComponentProps<typeof ScrollView>
andreialecu commented 3 years ago

I probably need to open a new issue, but...

I just tried v3 for the first time in Expo on my iPhone, and I'm seeing this:

IMG_BF8E8794E1CC-1

Also the snap example doesn't snap at all, and I cannot tap the tab titles to change pages (they don't do anything, only swiping does)

I'm on this branch, but it doesn't seem related to it. I could try main as well.

andreialecu commented 3 years ago

Actually it only snaps if momentum scrolling rests within the header.

But if I manually scroll and lift the finger with the header partly visible it doesn't snap. And the scroll positions of the other tabs are not synced, so they end up like the screenshot above.

PedroBern commented 3 years ago

59 is pointing the same problems 😞 everything is perfect here on my android. Didn't test on iOS so I couldn't tell. Do you have these bugs on android too?

andreialecu commented 3 years ago

Didn't test Android. It was quite tricky to get all of these right on iOS on the initial version I contributed to.

In its current state it seems a bit broken on iOS unfortunately :(

There are some other issues as well, such as overscrolling not working any more. Overscrolling is somewhat important to make scrollviews feel native on iOS, otherwise they just abruptly stop at the end, without the nice bounce.

andreialecu commented 3 years ago

I'll try to take a look at some of these over the weekend.

PedroBern commented 3 years ago

😞 I'm sorry to hear that. My mac mini is arriving in 2 weeks 🎉 so I will be able to investigate further, but as always, all the help is appreciated. Also, if we can't make it better, we can revert to v2...

alexpchin commented 3 years ago

@PedroBern I'm sure we can get v3 to work. I think a lot of issues are connected together?

PedroBern commented 3 years ago

@PedroBern I'm sure we can get v3 to work. I think a lot of issues are connected together?

Finally, with #65 or #67, we are getting there 🎉

andreialecu commented 3 years ago

I'd like to give this another try, but looks like it needs to be rebased.

PedroBern commented 3 years ago

yes sure, also I saw it is broken with diffClamp, don't know why. I need to go away right now and will take some hours to get back, then I resolve all these conflicts.

PedroBern commented 3 years ago

@andreialecu it's up to date now. I need to update the example app to have 2 screens, one with react-navigation examples and the other without it. Both screens with the same set of demos/examples.

PedroBern commented 3 years ago

@andreialecu see this note on going back

andreialecu commented 3 years ago

Seems this is a bigger breaking change than I anticipated. Will need to go through the example and docs.

There are a bunch of things missing that I noticed at first glance:

sceneContainerStyle
Screenshot 2021-02-01 at 21 54 06
tabBarLabel
Screenshot 2021-02-01 at 21 54 24
andreialecu commented 3 years ago

Actually all the props of Navigator seem to be missing. Including the other one visible in the screenshot (collapsibleOptions)

andreialecu commented 3 years ago

Ok, I see they're direct properties. Except animatedValue which I wasn't able to find yet. 🤔

PedroBern commented 3 years ago

It's brand new, has nothing to do with the v1/v2, I have built from the ground...

PedroBern commented 3 years ago

There isn't animatedValue. This may be a problem, since we have many shared values now, instead of the animatedValue.

The solution can be making some shared values controlled, just like refs. But it will be a change in the raw tabs, not the navigator.

andreialecu commented 3 years ago

I know that - thought it would keep the API more or less though 🙂

I'm leaving some more comments here as I go:

It appears that when changing Animated.FlatList into TabsNavigator.FlatList some problems appear:

Animated.FlatList
Screenshot 2021-02-01 at 22 23 51
TabsNavigator.FlatList
Screenshot 2021-02-01 at 22 24 51

item gets typed as unknown. ref doesn't seem to exist.

PedroBern commented 3 years ago

ref doesn't seem to exist.

This ref will be the same as in the refMap, you already have it.

item gets typed as unknown

Try passing the type explicitly to the flatlist <TabsNavigator.FlatList<ItemType> ... />, should work, but it also should work implicity 🤔

andreialecu commented 3 years ago

This ref will be the same as in the refMap, you already have it.

Seems it might be a little tricky to pass it down since the refMap is defined in the parent, and these are separate individual components. 🤔

It's likely I'm doing things wrong though, it's late here. Will need to revisit this when I'm less tired. Will take another look tomorrow. 🙂

PedroBern commented 3 years ago

Seems it might be a little tricky to pass it down since the refMap is defined in the parent, and these are separate individual components. 🤔

You can get from the context useTabsContext 🙂

andreialecu commented 3 years ago

Try passing the type explicitly to the flatlist <TabsNavigator.FlatList<ItemType> ... />, should work, but it also should work implicity 🤔

This didn't seem to work btw. Tried <any> without success. I didn't look to deeply into it, but it's possible something is wrong with the types generated by bob.

yfunk commented 3 years ago

Just out of curiosity, whats the current status on react-navigation integration for v4? Is it still something you are planning to add?

PedroBern commented 3 years ago

@yfunk not anytime soon, maybe in the future. If you want to work on a PR would be awesome :) As a starting point you can see the createCollapsibleNavigator from this branch, but keep in mind it was built on top of v3, so it needs an update.