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

Version 4.0 with api refactoring and dynamic tabs #93

Merged
merged 86 commits into from Feb 9, 2021

Conversation

andreialecu
Copy link
Collaborator

@andreialecu andreialecu commented Feb 3, 2021

Third alternative to #90 or #91 😆

Introduces a <Tabs.Tab /> component for defining the tabs.

      <Tabs.Container ref={ref} headerHeight={HEADER_HEIGHT} {...props}>
        <Tabs.Tab name="article">
          <Article />
        </Tabs.Tab>
        <Tabs.Tab name="albums">
          <Albums />
        </Tabs.Tab>
        <Tabs.Tab name="contacts">
          <Contacts emptyContacts={emptyContacts} />
        </Tabs.Tab>
      </Tabs.Container>

I think I like this the best personally. Seems most idiomatic with React practices in general.

Another benefit is that there's no more need to define the tabs twice (and to make sure the order matches).

@andreialecu
Copy link
Collaborator Author

andreialecu commented Feb 3, 2021

This may have an additional benefit that it would make it possible to add/remove tabs at runtime dynamically. I'm not sure if the dynamic part works, because I haven't tested it. But if it doesn't, it should be fixable.

@andreialecu
Copy link
Collaborator Author

Also, this is a breaking change so it would need to bump the package to 4.0.

Copy link
Owner

@PedroBern PedroBern left a comment

Choose a reason for hiding this comment

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

These changes are so awesome 🎉 🚀 Makes the API much more idiomatic, I really like it.

Just one concern, how do we get the ref from a parent component? Maybe a simple solution is to somehow map the refMap to an object inside the useImperativeHandle. So we can access like this: ref.current.article.current... or ref.current.tabs.article.current....

Would be awesome if it were possible to pass props to the Tab and send it to TabBarComponent or HeaderComponent. Just like react-navigation. But that's another story...

@PedroBern
Copy link
Owner

I'm thinking about how could we achieve an expo preview for PRs other than mine. The github secrets can't be shared between forks, so seems there is no right solution for this. Would be great if it were possible. Right now I need to fetch every PR and test locally.

@PedroBern
Copy link
Owner

PedroBern commented Feb 3, 2021

@andreialecu we can add something like this:

// utility hook
const useTabsOptions = (children: typeof Tab[] | typeof Tab) => {
  const options = React.useMemo(() => {
    const tabsOptions = {}
    React.Children.forEach(children, (element, index) => {
      const { options, name } = element.props
      tabsOptions[name] = options
    })
    return tabsOptions
  }, [children])
  return options
}

// inside the Container:

// extract the props passed to each Tab component and map to the tab names
const tabOptions = useTabsOptions(children)

// pass the tab options to the TabBar
<TabBarComponent options={tabOptions} ... />

And then use like this:

      <Tabs.Container ref={ref} headerHeight={HEADER_HEIGHT} {...props}>

        // direct props
        <Tabs.Tab name="article" options={{ label: ...}}>
          <Article />
        </Tabs.Tab>

        // or options
        <Tabs.Tab name="albums" options={{ label: ... }}>
          <Albums />
        </Tabs.Tab>

      </Tabs.Container>

It will grab the props before rendering the tab bar. We can pass it to the header too. In this example the TabBar would receive:

{
    article: { label: ... },
    albums: { label: ... }
} 

@PedroBern PedroBern added the preview Trigger expo preview action label Feb 3, 2021
@PedroBern
Copy link
Owner

PedroBern commented Feb 3, 2021

I have just added the options 🚀 Please review and make necessary changes if needed, including the naming conventions.

tried the preview after having commits from me, but still don't work

@PedroBern PedroBern removed the preview Trigger expo preview action label Feb 3, 2021
src/Tab.tsx Outdated
@@ -2,9 +2,9 @@ import React from 'react'

import { ParamList, TabOptions } from './types'

export interface TabProps<T extends ParamList> {
export type TabProps<T extends ParamList> = {
Copy link
Owner

@PedroBern PedroBern Feb 3, 2021

Choose a reason for hiding this comment

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

I changed to type instead of interface because the docs script doesn't like interfaces, and doesn't matter anyway.

src/hooks.tsx Outdated
}

export const useTabOptions = <T extends ParamList>(
children: React.ReactElement<TabProps<T>>
Copy link
Owner

Choose a reason for hiding this comment

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

this type can probably be narrowed down.

src/types.ts Outdated
label?: string
}

export type FinalTabOptions<T extends ParamList> = Record<
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have a better name for this? FinalTabOptions is awkward.

@andreialecu
Copy link
Collaborator Author

I'm working on this at the moment btw. Will report back in a few.

@andreialecu andreialecu marked this pull request as ready for review February 4, 2021 13:45
@dan-fein
Copy link

dan-fein commented Feb 9, 2021

I'm not using in a production app so I don't mind if it's incomplete, happy to report bugs here. What's the best way to use these latest changes?

@andreialecu
Copy link
Collaborator Author

@danielfein I already replied to your previous comment here with instructions. Scroll up a bit. Feedback would be greatly appreciated.

There are some issues on android with the latest commit which are being worked on (related to snapping and flatlists)

@alexpchin
Copy link
Contributor

@danielfein I already replied to your previous comment here with instructions. Scroll up a bit. Feedback would be greatly appreciated.

There are some issues on android with the latest commit which are being worked on (related to snapping and flatlists)

Hi @andreialecu did as you said to use your version in my project. However, it seems like the prepack script doesn't automatically run. When adding this to my postinstall script.

npm i --prefix ./node_modules/react-native-collapsible-tab-view && npm run prepack --prefix ./node_modules/react-native-collapsible-tab-view

I see these errors:

> react-native-collapsible-tab-view@3.7.1 prepack /Users/alexchin/ReactNative/iynk-react-app/node_modules/react-native-collapsible-tab-view
> bob build

ℹ Building target commonjs
ℹ Cleaning up previous build at lib/commonjs
ℹ Compiling 15 files in src with babel
✓ Wrote files to lib/commonjs
ℹ Building target module
ℹ Cleaning up previous build at lib/module
ℹ Compiling 15 files in src with babel
✓ Wrote files to lib/module
ℹ Building target typescript
ℹ Cleaning up previous build at lib/typescript
ℹ Generating type definitions with tsc
src/helpers.tsx:18:14 - error TS2742: The inferred type of 'AnimatedFlatList' cannot be named without a reference to 'react-native-collapsible-tab-view/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.

18 export const AnimatedFlatList = Animated.createAnimatedComponent(FlatList)
                ~~~~~~~~~~~~~~~~

src/hooks.tsx:34:17 - error TS2742: The inferred type of 'useContainerRef' cannot be named without a reference to 'react-native-collapsible-tab-view/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.

34 export function useContainerRef() {
                   ~~~~~~~~~~~~~~~

src/hooks.tsx:481:17 - error TS2742: The inferred type of 'useSharedAnimatedRef' cannot be named without a reference to 'react-native-collapsible-tab-view/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.

481 export function useSharedAnimatedRef<T extends RefComponent>(
                    ~~~~~~~~~~~~~~~~~~~~

src/index.tsx:27:14 - error TS2742: The inferred type of 'Tabs' cannot be named without a reference to 'react-native-collapsible-tab-view/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.

27 export const Tabs = {
                ~~~~


Found 4 errors.

Also:

  • This package has been renamed to 'react-native-builder-bob'. Please use it instead.

Propose releasing as @next ?

@andreialecu
Copy link
Collaborator Author

react-native-collapsible-tab-view-v3.7.1.tgz.zip

I ran pack on the current tip of this PR. Extract the zip, and add it as file:react-native-collapsible-tab-view-v3.7.1.tgz (or check npm's documentation)

(or clone this PR, and run yarn && yarn pack to generate the file above)

@andreialecu
Copy link
Collaborator Author

andreialecu commented Feb 9, 2021

There is a known issue with RefreshControl + FlatList + Snapping on Android (iOS works fine). The issue also affects the current published version though.

We believe it's a reanimated bug: software-mansion/react-native-reanimated#1703

@alexpchin
Copy link
Contributor

alexpchin commented Feb 9, 2021

Hi @andreialecu I'm now setup with the new version in my project. Just getting to grips with the new setup but currently when I scroll up on iOS, the header does not scroll with the list. Also I'm seeing [Object Map Iterator] in the TabBar as per:

I will continue to have a look. (I previously had a few pages to update to the new version).

@andreialecu
Copy link
Collaborator Author

andreialecu commented Feb 9, 2021

Make sure to use the scrollview or flatlist exported from this package. Not the react native ones.

If the problem persists, a code sample of how you're using it would be helpful.

@PedroBern PedroBern merged commit 03e7202 into PedroBern:main Feb 9, 2021
@PedroBern PedroBern mentioned this pull request Feb 9, 2021
3 tasks
@PedroBern PedroBern linked an issue Feb 9, 2021 that may be closed by this pull request
3 tasks
@andreialecu andreialecu deleted the refactor-apicleanup3 branch October 5, 2021 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants