grahammendick / navigation

Scene-Based Navigation for React and React Native
https://grahammendick.github.io/navigation/
Apache License 2.0
571 stars 40 forks source link

TabBarItem font props won't work #750

Open belowfrog opened 9 months ago

belowfrog commented 9 months ago

TabBarItem component's font-related props will not take effect if it is wrapped in another component and placed within the TabBar component.

for example:

<TabBar primary={true}>
  <WrappedTabbarItem />
</TabBar>

const WrappedTabbarItem = () => (
  <TabBarItem title="Assets" fontSize={12} fontWeight="600">
  ...
  </TabBar>
)

I suggest that it is best to place the font property on the TabBar itself because it cannot be set individually on TabBarItem and will affect each other's font properties.

<TabBar primary={true} fontSize={12} fontWeight="600">

belowfrog commented 9 months ago

you could test with the twitter example, just replace Tabs.js with this:


import React, {useState, useMemo, useContext} from 'react';
import {Platform} from 'react-native';
import {StateNavigator} from 'navigation';
import {NavigationHandler, NavigationContext} from 'navigation-react';
import {
  NavigationStack,
  Scene,
  TabBar,
  TabBarItem,
  NavigationBar,
} from 'navigation-react-native';
import Home from './Home';
import Notifications from './Notifications';
import Tweet from './Tweet';
import Timeline from './Timeline';
import {getNotifications} from './data';

const useStateNavigator = () => {
  const {stateNavigator} = useContext(NavigationContext);
  return useMemo(() => new StateNavigator(stateNavigator), []);
};

export default () => {
  const [notified, setNotified] = useState(false);
  const notificationsNavigator = useStateNavigator();
  return (
    <>
      <NavigationBar hidden={true} />
      <TabBar
        primary={true}
        barTintColor={Platform.OS === 'android' ? null : 'rgb(247,247,247)'}
        selectedTintColor={Platform.OS === 'android' ? '#1da1f2' : null}>
        <WrappedTabBarItem />
        <TabBarItem
          title="Notifications"
          image={require('./notifications.png')}
          badge={!notified ? '' : null}
          onPress={() => {
            setNotified(true);
          }}>
          {Platform.OS === 'ios' ? (
            <NavigationHandler stateNavigator={notificationsNavigator}>
              <NavigationStack>
                <Scene stateKey="notifications">
                  <Notifications />
                </Scene>
                <Scene stateKey="tweet">
                  <Tweet />
                </Scene>
                <Scene stateKey="timeline">
                  <Timeline />
                </Scene>
              </NavigationStack>
            </NavigationHandler>
          ) : (
            <Notifications />
          )}
        </TabBarItem>
      </TabBar>
    </>
  );
};

const WrappedTabBarItem = () => {
  const homeNavigator = useStateNavigator();

  return (
    <TabBarItem title="Home" fontSize={12} fontWeight="800">
      {Platform.OS === 'ios' ? (
        <NavigationHandler stateNavigator={homeNavigator}>
          <NavigationStack>
            <Scene stateKey="home">
              <Home />
            </Scene>
            <Scene stateKey="tweet">
              <Tweet />
            </Scene>
            <Scene stateKey="timeline">
              <Timeline />
            </Scene>
          </NavigationStack>
        </NavigationHandler>
      ) : (
        <Home />
      )}
    </TabBarItem>
  );
};
grahammendick commented 9 months ago

Hey, thanks for getting in touch with such a clear write-up.

The TabBar component does loop through its children looking for props (and some other things). Not ideal because it stops working if you wrap them in another component, like you said.

I still prefer it to moving props up to the top level. If all props are on the TabBar then it’s much messier to support different fonts for different tabs, for example.

What about if I added some validation so you get an error if the direct children aren’t TabBarItems?

belowfrog commented 9 months ago

yeah, it would be more clear to throw an error out for the wrong children.