search

grahammendick / navigation

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

Reg ToolbarAndroid #309

Closed arunmenon1975 closed 5 years ago

arunmenon1975 commented 5 years ago

React's lean core effort is going full steam and now ToolbarAndroid is no longer available in the codebase though, as of today, the documentation does exist on the main doc site.

Examples in this repo that use ToolbarAndroid, like the Twitter sample, currently call it from react-native and this is bound to break in versions of react-native higher that v61, as i found out as well in my project.

Please let me know if you are considering shipping an Android variant sometime in the future.

Pitches for having it in the library:

To get a feel of Android native module integration with the router, my initial attempts at a crude/copy-paste integration results in errors currently. I get the dreaded 'Invariant Violation' error ("expected a string for built-in components...you likely forgot to export your component from the file it is defined in..."). I think i did the right things, at least after cross-checking with how the other modules - like the tabs - were integrated, but somehow i keep getting the error i mentioned above.

Steps i undertook:

Im still not to sure if just pulling in the code as-is from an earlier version somehow messes up the licensing bit even if the comments and attributions are intact. My guess is it should be ok for FOSS projects.

This original ToolbarAndroid module is quite comprehensive and if indeed you are looking at having a common Toolbar component across platforms across both the platforms, some rationalization or simplification may be warranted i think.

If you are not too keen on having it integrated i can always use the same approach but have it exposed via NativeModules. It perhaps may work. Or may not. Or maybe totally simplify it just for for my needs -> a back button, a title and perhaps a menu button/icon

UPDATE:

I have to make a few changes to .tsx files as well. Will update here after completing those.

arunmenon1975 commented 5 years ago

Update: i no longer receive errors. I updated the tsx files and added the new build to my project.

However, the default example 

<ToolbarAndroid
      logo={require('./app_logo.png')}
     style={{flex:1, backgroundColor: "blue"}}  /*my addition*/
      title="AwesomeApp"
      actions={[{title: 'Settings', icon: require('./icon_settings.png'), show: 'always'}]}
      onActionSelected={this.onActionSelected} 
/>

throws an error at the logo property even after i changed it to a valid local path. The error: image

If i comment that attribute out, there is no error but i only get the title to appear in the Toolbar. The actions icon/title do not appear.

grahammendick commented 5 years ago

Have you kept the resolveAssetSource call in the JavaScript? 

import {Image} from 'react-native'

if (this.props.logo) {
  nativeProps.logo = Image.resolveAssetSource(this.props.logo);
}
grahammendick commented 5 years ago

I'm excited by your idea of bringing the ToolbarAndroid component into the Navigation router. Let me know how you get on with your copy/paste experiment and we can discuss the next steps

arunmenon1975 commented 5 years ago

Have you kept the resolveAssetSource call in the JavaScript?

import {Image} from 'react-native'

if (this.props.logo) {
  nativeProps.logo = Image.resolveAssetSource(this.props.logo);
}

In my enthusiasm i forgot requisite JS changes considering this is a react-native component clone and thus would be having its implementation details separately and possibly alongside other react native components.

I did take a look. This can turn out tricky simply because JS methods used by ToolbarAndroid to integrate functionality like images/icons (including parsing the URI and for getting the dimensions like width/height) and onPress are tied into common methods used in the core react-native codebase. Copy/pasting this would mean having a companion utility .ts/x file (or embedded within the main .tsx file) that has the requisite methods in them. Even if this works out ok, there is a possibly of code duplication between methods introduced by react-native and similar methods implemented in the router. Since the router doesn't ship with react-native it cannot be built if i attempt a relative path import for the required JS files.

I currently see 2 options:

arunmenon1975 commented 5 years ago

I'm excited by your idea of bringing the ToolbarAndroid component into the Navigation router. Let me know how you get on with your copy/paste experiment and we can discuss the next steps

Thanks for the go-ahead. It will indeed be a useful addition to the router if things go well.

My thoughts currently:

What i am attempting:

...
const React = require('react');
const UIManager = require('../../ReactNative/UIManager');
const ToolbarAndroidNativeComponent = require('./ToolbarAndroidNativeComponent');
const resolveAssetSource = require('../../Image/resolveAssetSource');
import type {SyntheticEvent} from '../../Types/CoreEventTypes';
import type {ImageSource} from '../../Image/ImageSource';
import type {ColorValue} from '../../StyleSheet/StyleSheetTypes';
import type {ViewProps} from '../View/ViewPropTypes';
import type {NativeComponent} from '../../Renderer/shims/ReactNative';
...

Im still unsure whether to embed code within this file or have a companion file.

grahammendick commented 5 years ago

For your copy/paste experiment I don’t think we should remove any functionality. We should verify that we can get all the functionality working. That doesn’t mean that we’ll bring all the functionality into the Navigation router but it will help us with decision-making to know that we can.

The JavaScript doesn’t look too scary. We should be able to bring all that over without creating any utilities. For example, the resolveAssetSource is static function on Image so you can call it like this:

import {Image} from 'react-native'

if (this.props.logo) {
  nativeProps.logo = Image.resolveAssetSource(this.props.logo);
}

You shouldn’t need to import the UIManager. It’s used for a string lookup which we can hard-code instead.

You don’t need to copy over any of the type imports or type related code because that’s all related to flow.

arunmenon1975 commented 5 years ago

Thanks for the suggestions. I made changes, as much as i understood, but i still get the same error. This is the current code that i have (please disregard the types - it was just for personal testing):

import React, { SyntheticEvent } from 'react'
import { Image, requireNativeComponent, Platform, NativeComponent } from 'react-native'

type ImageURISource = {
  uri?: string,
  bundle?: string,
  method?: string,
  headers?: Object,
  body?: string,
  cache?: ('default' | 'reload' | 'force-cache' | 'only-if-cached'),
  width?: number,
  height?: number,
  scale?: number
};
export type ImageSource = ImageURISource | number | Array<ImageURISource>;
type Action = {
  title: string,
  icon?: ImageSource,
  show?: 'always' | 'ifRoom' | 'never',
  showWithText?: boolean
};

type ToolbarAndroidChangeEvent = {
  nativeEvent: {
    position: number
  }
};
type NativeProps = {
  onSelect: (event: ToolbarAndroidChangeEvent) => any,
  nativeActions?: Array<Action>
};
type ColorValue = null | string;
type ToolbarAndroidProps = {
  //...ViewProps, ...NativeProps,
  onSelect: (event: ToolbarAndroidChangeEvent) => any,
  nativeActions?: Array<Action>,
  actions?: Array<Action>,
  logo?: ImageSource,
  navIcon?: ImageSource,
  onActionSelected?: (position: number) => void,
  onIconClicked?: () => void,
  overflowIcon?: ImageSource,
  subtitle?: string,
  subtitleColor?: ColorValue,
  title?: string,
  titleColor?: ColorValue,
  contentInsetStart?: number,
  contentInsetEnd?: number,
  rtl?: boolean,
  testID?: string
};

//type NativeToolbarAndroidProps = <NativeComponent<ToolbarAndroidProps>>;

const ToolbarAndroidNativeComponent = ((requireNativeComponent('ToolbarAndroid')));
const resolveAssetSource = Image.resolveAssetSource;

type Props = {
  // ...ToolbarAndroidProps,
  forwardedRef: React.Ref<typeof ToolbarAndroidNativeComponent>
};

class ToolbarAndroid extends React.Component<Props & ToolbarAndroidProps> {
  _onSelect = (event) => {
    const position = event.nativeEvent.position;
    if (position === -1) {
      this.props.onIconClicked && this
        .props
        .onIconClicked();
    } else {
      this.props.onActionSelected && this
        .props
        .onActionSelected(position);
    }
  };

  render() {
    const {
      onIconClicked,
      onActionSelected,
      forwardedRef,
      ...otherProps
    } = this.props;

    const nativeProps = {
      ...otherProps
    };

    if (this.props.logo) {
      nativeProps.logo = resolveAssetSource(this.props.logo);
    }

    if (this.props.navIcon) {
      nativeProps.navIcon = resolveAssetSource(this.props.navIcon);
    }

    if (this.props.overflowIcon) {
      nativeProps.overflowIcon = resolveAssetSource(this.props.overflowIcon);
    }

    if (this.props.actions) {
      const nativeActions = [];
      for (let i = 0; i < this.props.actions.length; i++) {
        const action = {
          icon: this.props.actions[i].icon,
          show: this.props.actions[i].show
        };
        const ShowAsAction = ['always', 'ifRoom', 'never']
        if (action.icon) {
          action.icon = resolveAssetSource(action.icon);
        }
        if (action.show) {
          action.show = ShowAsAction[action.show];
        }
        /* if (action.show) {
          action.show = UIManager.getViewManagerConfig(
            'ToolbarAndroid',
          ).Constants.ShowAsAction[action.show];
        } */

        nativeActions.push({
          ...this.props.actions[i],
          ...action
        });
      }

      nativeProps.nativeActions = nativeActions;
    }

    return (<ToolbarAndroidNativeComponent
      onSelect={this._onSelect}
      {...nativeProps}
      ref={forwardedRef} />);
  }
}

const ToolbarAndroidToExport = React.forwardRef((props, forwardedRef, ) => {
  return <ToolbarAndroid {...props} forwardedRef={forwardedRef} />;
});

/* import { requireNativeComponent, Platform } from 'react-native';
 */
// export default Platform.OS === 'ios' ?
// requireNativeComponent('ToolbarAndroid', null) : () => null;
export default ToolbarAndroidToExport;

and after a build, i attempt:

<ToolbarAndroid title="Home" logo={require('../../resources/media/images/icons/calendar-body.png')} style={{flex:1}} />

I think i definitely am nearer a working solution though i still get the same error.

[I still have flaky internet despite having 3 providers. A fresh build in react-native versions upwards of v60 seems to always require internet connectivity for some reason, perhaps due to jettifier not sure. Thus i have to have a stable enough connection through the build duration for viewing my changes on the emulator or it just seems to idle around midway at some process or the other. And so my changes do take time to verify]

grahammendick commented 5 years ago

I've simplified the code so that we can see if the problem is with the logo prop or not. Can you try this out and let me know what happens. Send along any error message you get, please.

import React from 'react'
import { Image, requireNativeComponent } from 'react-native'

const ToolbarAndroidNativeComponent = requireNativeComponent('ToolbarAndroid');

export default ({logo}) => (
  <ToolbarAndroidNativeComponent logo={Image.resolveAssetSource(logo)} />)
);
arunmenon1975 commented 5 years ago

I get a build error:

[16:10:18] Using gulpfile ~/Sites/github/navigation/navigation/gulpfile.js
[16:10:18] Starting 'build'...
[16:10:18] Starting 'buildNavigation'...
[16:10:18] Starting 'buildNavigationReact'...
[16:10:18] Starting 'buildNavigationReactMobile'...                                    [16:10:18] Starting 'buildNavigationReactNative'...
/Users/arunmenon/Sites/github/navigation/navigation/NavigationReactNative/src/ToolbarAn
droid.tsx(8,1): error TS1128: Declaration or statement expected.
[16:10:19] 'buildNavigationReactNative' errored after 1.18 s
[16:10:19] Error: There were TypeScript errors transpiling
    at Object.transform (/Users/arunmenon/Sites/github/navigation/navigation/node_modules/rollup-plugin-typescript/dist/rollup-plugin-typescript.cjs.js:302:11)
    at /Users/arunmenon/Sites/github/navigation/navigation/node_modules/rollup/dist/rollup.js:17680:41
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
[16:10:19] 'build' errored after 1.19 s
[16:10:19] The following tasks did not complete: buildNavigation, buildNavigationReact, buildNavigationReactMobile
[16:10:19] Did you forget to signal async completion?
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! navigation-@1.0.0 build: `gulp build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the navigation-@1.0.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/arunmenon/.npm/_logs/2019-10-14T10_40_19_480Z-debug.log

And the log:

0 info it worked if it ends with ok
1 verbose cli [
1 verbose cli   '/usr/local/Cellar/node/12.9.0/bin/node',
1 verbose cli   '/usr/local/bin/npm',
1 verbose cli   'run',
1 verbose cli   'build'
1 verbose cli ]
2 info using npm@6.10.3
3 info using node@v12.9.0
4 verbose run-script [ 'prebuild', 'build', 'postbuild' ]
5 info lifecycle navigation-@1.0.0~prebuild: navigation-@1.0.0
6 info lifecycle navigation-@1.0.0~build: navigation-@1.0.0
7 verbose lifecycle navigation-@1.0.0~build: unsafe-perm in lifecycle true
8 verbose lifecycle navigation-@1.0.0~build: PATH: /usr/local/lib/node_modules/npm/node_modules/npm-lifecycle/node-gyp-bin:/Users/arunmenon/Sites/github/navigation/navigation/node_modules/.bin:/Users/arunmenon/flutter/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/MacGPG2/bin:/Users/arunmenon/flutter/bin:/usr/local/sbin:/usr/local/mysql/bin:/Users/arunmenon/Library/Android/sdk/tools:/Users/arunmenon/Library/Android/sdk/tools/bin:/Users/arunmenon/Library/Android/sdk/platform-tools:/Applications/Visual Studio Code.app/Contents/Resources/app/bin:/usr/local/mysql/bin:/Users/arunmenon/Library/Android/sdk/tools:/Users/arunmenon/Library/Android/sdk/tools/bin:/Users/arunmenon/Library/Android/sdk/platform-tools:/usr/local/mysql/bin:/Applications/Visual Studio Code.app/Contents/Resources/app/bin
9 verbose lifecycle navigation-@1.0.0~build: CWD: /Users/arunmenon/Sites/github/navigation/navigation
10 silly lifecycle navigation-@1.0.0~build: Args: [ '-c', 'gulp build' ]
11 silly lifecycle navigation-@1.0.0~build: Returned: code: 1  signal: null
12 info lifecycle navigation-@1.0.0~build: Failed to exec build script
13 verbose stack Error: navigation-@1.0.0 build: `gulp build`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/npm-lifecycle/index.js:326:16)
13 verbose stack     at EventEmitter.emit (events.js:209:13)
13 verbose stack     at ChildProcess.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/npm-lifecycle/lib/spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:209:13)
13 verbose stack     at maybeClose (internal/child_process.js:1021:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)
14 verbose pkgid navigation-@1.0.0
15 verbose cwd /Users/arunmenon/Sites/github/navigation/navigation
16 verbose Darwin 18.7.0
17 verbose argv "/usr/local/Cellar/node/12.9.0/bin/node" "/usr/local/bin/npm" "run" "build"
18 verbose node v12.9.0
19 verbose npm  v6.10.3
20 error code ELIFECYCLE
21 error errno 1
22 error navigation-@1.0.0 build: `gulp build`
22 error Exit status 1
23 error Failed at the navigation-@1.0.0 build script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 1, true ]
arunmenon1975 commented 5 years ago

..i think an additional closing parenthesis is the issue. The build is working fine now after i removed it. Let me try and rebuild my project. Slight likelihood of delay but i will update here once its complete

arunmenon1975 commented 5 years ago

The build finally completed. I get the same error.

Just to be double-sure let me clean and re-build - i didn't do the clean-and-build routine since i was worried about the build taking forever.

Will update back with this result as well.

arunmenon1975 commented 5 years ago

The build finally completed. I get the same error.

Just to be double-sure let me clean and re-build - i didn't do the clean-and-build routine since i was worried about the build taking forever.

Will update back with this result as well.

Same error with a clean-and-build task as well.

grahammendick commented 5 years ago

The only reliable way I've found to do a 'clean-and-build' is to delete the navigation-react-native folder from node_modules. Then do a react-native start and react-native run-android. This should give the error that navigation-react-native is missing. Then copy the newly built navigation-react-native into node_modules and do a react-native start and react-native run-android again.

Can you try that because I'm surprised the current code gives the 'Double to ReadableMap' error. The call to Image.resolveAssetSource converts the number into a map.

grahammendick commented 5 years ago

If you're only making JavaScript changes (like we are here), then there's a quicker way to test them without having to mess around with clean and rebuilds. Copy just the newly built navigation.react.native.js into the node_modules/navigation-react-native folder and then reload the running app - by pressing Reload on the Dev Menu (or double tap R)

grahammendick commented 5 years ago

If you debug the JavaScript do you see that Image.resolveAssetSource turns the number into json?

arunmenon1975 commented 5 years ago

The only reliable way I've found to do a 'clean-and-build' is to delete the navigation-react-native folder from node_modules. Then do a react-native start and react-native run-ios. This should give the error that navigation-react-native is missing. Then copy the newly built navigation-react-native into node_modules and do a react-native start and react-native run-ios again.

Can you try that because I'm surprised the current code gives the 'Double to ReadableMap' error. The call to Image.resolveAssetSource converts the number into a map.

Same error. I did get the 'could not file package.json...' error etc and followed the exact steps.

arunmenon1975 commented 5 years ago

If you're only making JavaScript changes (like we are here), then there's a quicker way to test them without having to mess around with clean and rebuilds. Copy just the newly built navigation.react.native.js into the node_modules/navigation-react-native folder and then reload the running app - by pressing Reload on the Dev Menu (or double tap R)

Ok. I will try this in subsequent builds (with a fallback to a full build if things dont work)

arunmenon1975 commented 5 years ago

If you're only making JavaScript changes (like we are here), then there's a quicker way to test them without having to mess around with clean and rebuilds. Copy just the newly built navigation.react.native.js into the node_modules/navigation-react-native folder and then reload the running app - by pressing Reload on the Dev Menu (or double tap R)

Ok. I will try this in subsequent builds (with a fallback to a full build if things dont work)

Strangely, after doing a build, i get the following javascript entry for ToolbarAndroid in the file ./build/npm/navigation-react-native/navigation.react.native.js :

var ToolbarAndroid = Platform.OS === 'android' ? requireNativeComponent('ToolbarAndroid', null) : function () { return null; };

but i have this is in ./NavigationReactNative/src/ToolbarAndroid.tsx:

import React from 'react'
import { Image, requireNativeComponent } from 'react-native'

const ToolbarAndroidNativeComponent = requireNativeComponent('ToolbarAndroid');

export default ({logo}) => (
  <ToolbarAndroidNativeComponent logo={Image.resolveAssetSource(logo)} />)

EDIT: ..in context, and this in my ./NavigationReactNative/src/NavigationReactNative.ts

import { Platform } from 'react-native';
import NavigationStack from './NavigationStack';
import Scene from './Scene';
import NavigationBarIOS from './NavigationBarIOS';
import LeftBarIOS from './LeftBarIOS';
import RightBarIOS from './RightBarIOS';
import BarButtonIOS from './BarButtonIOS';
import SearchBarIOS from './SearchBarIOS';
import TabBar from './TabBar';
import TabBarItem from './TabBarItem';
import SharedElementAndroid from './SharedElementAndroid';
import TitleBarIOS from './TitleBarIOS';
import BackHandlerContext from './BackHandlerContext';
import ToolbarAndroid from './ToolbarAndroid';

var TabBarIOS = Platform.OS === 'ios' ? TabBar : () => null;
var TabBarItemIOS = Platform.OS === 'ios' ? TabBarItem : () => null;

export { NavigationStack, Scene, NavigationBarIOS, LeftBarIOS, RightBarIOS, BarButtonIOS, TitleBarIOS, SearchBarIOS, TabBar, TabBarItem, TabBarIOS, TabBarItemIOS, SharedElementAndroid, BackHandlerContext, ToolbarAndroid };
arunmenon1975 commented 5 years ago

ok, so i do see my change in ./build/dist/navigation.react.native.js

 var ToolbarAndroid = (function (_a) {
        var logo = _a.logo;
        console.log(logo);
        return (React__default.createElement(ToolbarAndroidNativeComponent, { logo: reactNative.Image.resolveAssetSource(logo) }));
    });
grahammendick commented 5 years ago

Is it appearing twice? Where is the other one coming from? It's probably ignoring our one and hitting the wrong one instead

arunmenon1975 commented 5 years ago

For updating changes, i am following the approach you had suggested in another issue here.

wrt the JS changes, in my local copy of the Navigation router repo the following is my approach:

Please do correct if i've missed something.

grahammendick commented 5 years ago

Have you left in some other code at the bottom of ToolbarAndroid.tsx?

arunmenon1975 commented 5 years ago

Have you left in some other code at the bottom of ToolbarAndroid.tsx?

No. I have it as:

import React from 'react'
import { Image, requireNativeComponent } from 'react-native'

const ToolbarAndroidNativeComponent = requireNativeComponent('ToolbarAndroid');

export default ({logo}) => {
  console.log(logo)
 return (<ToolbarAndroidNativeComponent logo={Image.resolveAssetSource(logo)} />)
}

I restarted my computer as well. I think ill refetch the Navigation repo and start again from a fresh copy. The changes are minimal currently so it should not be an issue.

arunmenon1975 commented 5 years ago

I did a complete refresh of the Navigation router files from the original repo and re-copied the toolbar changes over. There is no error but somehow the Toolbar isn't appearing. Logging the prop returns the following in my console:

{logo: 1
style: {flex: 1, width: 200, height: 100}
title: "Home"}

for my component declaration: <ToolbarAndroid title="Home" logo={require('../../resources/media/images/icons/calendar-body.png')} style={{flex:1, width: 200, height: 100}} />

grahammendick commented 5 years ago

Great, that's progress! If you're using the code I posted earlier then it's only sending the logo prop. It ignores your style prop so the Toolbar maybe has 0 height? Here's the updated component that sends on the other props together with the logo. Can you give that a try?

import React from 'react'
import { Image, requireNativeComponent } from 'react-native'

const ToolbarAndroidNativeComponent = requireNativeComponent('ToolbarAndroid');

export default ({logo,...props}) => (
  <ToolbarAndroidNativeComponent logo={Image.resolveAssetSource(logo)} {...props} />
);
arunmenon1975 commented 5 years ago

It works! And this time, post npm run package i only copied over the js file as you had suggested in one of the comments above. No issues. My style prop is picked up as well as the logo.

arunmenon1975 commented 5 years ago

It works! And this time, post npm run package i only copied over the js file as you had suggested in one of the comments above. No issues. My style prop is picked up as well as the logo.

In continuation and for context, this is my working code currently:

<View style={{flex: 1,
      justifyContent: 'flex-start',
      alignItems: 'stretch',
      backgroundColor: 'blue',
      }}>
       <ToolbarAndroid  title="Home" logo={require('../../resources/media/images/icons/arrow.png')} style={{ height: 100,  backgroundColor: "red"}}  />
grahammendick commented 5 years ago

Yippee! Are you interested in working on the PR that brings the Android Toolbar into the Navigation router? I've got an idea of how we should go about it. But before I spec it out I want to know if you’re interested. I should warn you that it will be a lot of work. Whatever you decide is fine, there’s no pressure either way.

arunmenon1975 commented 5 years ago

Yippee! Are you interested in working on the PR that brings the Android Toolbar into the Navigation router? I've got an idea of how we should go about it. But before I spec it out I want to know if you’re interested. I should warn you that it will be a lot of work. Whatever you decide is fine, there’s no pressure either way.

Definitely interested, thanks for the offer. I actually look at it this way: its not everyday i get opportunities like this that entails being involved in a core piece of a software tool. Please do let me know how you plan to go ahead with this. Im in fully. My only constraint is that the turnaround-time may not be fast since i have a network connectivity issue of late that seems to continue despite best efforts from my side. Another constraint is of course my limited experience so there is the possibility of having moments like 'did he really do that' or suchlike; i will try my best though to get it sailing smoothly.

grahammendick commented 5 years ago

That's fantastic! I'm so glad you're up for the job!

Don't worry about anything. The most important thing is that you're keen. We'll work through any problems together just like we've already been doing. It will be great working with you on such a nice piece of work. Thanks for bring the work to my attention in the first place!

I'm going to be out the rest of the day but I'll put together the plan shortly. In the meantime, I'd be interested to hear how you think we should bring the Android Toolbar into the Navigation router. Jot down any thoughts you have.

arunmenon1975 commented 5 years ago

And just for early reference, using most of the existing props:

<ToolbarAndroid  
        title="Home" 
        titleColor='yellow'
        subtitle="Lorem Ipsum"
        subtitleColor="gray"
        overflowIcon={require('../../resources/media/images/icons/menu-vertical-dots.png')} 
        contentInsetEnd={20}
        contentInsetStart={20}
        logo={require('../../resources/media/images/icons/menu-menu.png')} 
        style={{ height: 100,  backgroundColor: "red"}}  
        actions={[{title: 'Settings', icon: require('../../resources/media/images/icons/menu-settings.png'), show: 'always', showWithText: true}]}
        onActionSelected={_actionSelected}
        navIcon={require('../../resources/media/images/icons/arrow.png')} 
        onIconClicked={()=>Alert.alert('tapped')}
        />

the result looks like:

image

arunmenon1975 commented 5 years ago

That's fantastic! I'm so glad you're up for the job!

Don't worry about anything. The most important thing is that you're keen. We'll work through any problems together just like we've already been doing. It will be great working with you on such a nice piece of work. Thanks for bring the work to my attention in the first place!

I'm going to be out the rest of the day but I'll put together the plan shortly. In the meantime, I'd be interested to hear how you think we should bring the Android Toolbar into the Navigation router. Jot down any thoughts you have.

ok, ill post them in a separate comment in a short while.

grahammendick commented 5 years ago

My plans changed so I found some time to write a High-Level spec!

High-Level Idea

There’s already a NavigationBarIOS component but it only works on iOS 

<NavigationBarIOS title="Color" />

We should change this into a NavigationBar component that works on Android as well on iOS. On iOS the implementation stays the same (it uses the UINavigationBar). But on Android we’ll implement it as a Toolbar.

<NavigationBar title="Color" />

So, we’re not bringing React Native’s ToolbarAndroid component into the Navigation router. Instead, we’re implementing the Navigation router’s NavigationBar component on Android.

For the first release we’ll concentrate on the existing functionality of the NavigationBarIOS component only. So we won’t need most of the existing props on the ToolbarAndroid component.

Actions

To add buttons to the iOS navigation bar we use the Right/LeftBarIOS and BarButtonIOS components.

<NavigationBarIOS title="Color">
  <RightBarIOS>
    <BarButtonIOS title="Cancel" onPress={() => {
    }} />
  </RightBarIOS>
</NavigationBarIOS>

The equivalent of navigation bar buttons on Android are Menu Items. So we’ll make the Right/LeftBar and BarButton components work on Android by making them behave like the actions prop on the ToolbarAndroid component.

<NavigationBar title="Color">
  <RightBar>
    <BarButton title="Cancel" onPress={() => {
    }} />
  </RightBar>
</NavigationBar>

Title View

The title view can be customized on iOS using the TitleViewIOS component. Again, we’ll make this work on Android because the Toolbar supports child views.

Search View

There is a SearchBarIOS component that adds a search view into the navigation bar on iOS. Although there is an Android equivalent, I don’t think we should implement this for the first release. The iOS and Android search implementations are different enough that I think it needs a lot more investigation. So when we complete this PR the SearchBarIOS component will be the only component in the Navigation router that doesn’t have an Android equivalent.

I haven’t gone into any implementation details because I want to see what you think of the approach first. Once we’re both happy with the high-level idea then we can talk about how we’re going to implement it.

arunmenon1975 commented 5 years ago

Some thoughts on a toolbar solution:

Assuming there is no platform-independent toolbar in the works

Assuming there is platform-independent toolbar planned I guess this requires a lot more thought.

From my project perspective

I initially wanted to go a non-standard route to present my UI/UX since the decided premise of the solution was to be more fun-like and visual-heavy. There really wouldn’t be ‘standard’ components and UI like tabs or toolbars purposefully to sort of ‘remain different from most of the rest’. I then realised that since i am using RN, it would require some compromising especially on performance since it would all have to be JS-driven considering my skillsets. Or i will have to make a substantial leap and improve my code-writing skills in Java/Kotlin/ObjC/Swift and either write custom native modules or ditch RN entirely and use native supported languages for the complete app. After a lot of pondering i decided to go with RN while proactively trying and using as much native solutions as i could to help improve performance. Using Navigation router was the result of this decision incidentally since it sort of had the right balance.

My ideal native toolbar on Android currently would be (beyond certain features already available now):

All things considered, in my opinion, the best short/medium-term solution would be to continue with integrating the original RN ToolbarAndroid with a little more than what is considered as base requirements of a toolbar in a normal standard Android app today: a backbutton interactable, a title/subtitle, a logo and a menu intractable. In other words, a toned-down ToolbarAndroid as it exists now.

arunmenon1975 commented 5 years ago

My plans changed so I found some time to write a High-Level spec!

Ok. Just fyi, and to reduce confusion since my comment came in late, I just saw this message now after posting my own comment (the 5th or 6th attempt went through). Please disregard my comments for now.

Am reading through your high level specs and will revert shortly

arunmenon1975 commented 5 years ago

My plans changed so I found some time to write a High-Level spec!

High-Level Idea

I definitely like this idea: custom cross-platform module with (almost full) feature parity with one of the existing implementations. This is circuitous(from an effort and time perspective) but a more robust and long term solution. Or actually not as circuitous since half the work is already done in the form of the Ios equivalent already being available. Integrating the existing ToolbarAndroid could have been had but i guess its more a stop-gap effort.

So, in my understanding, a more or less full featured bare-basic toolbar (left button, title, right button ) in the Navigation router would read:

<NavigationBar title="Color">
  <LeftBar>
    <BarButton title="Back" onPress={() => {
    }} />
  </LeftBar>

  <RightBar>
    <BarButton title="Cancel" onPress={() => {
    }} />
  </RightBar>
</NavigationBar>

with the possibility of being enhanced with a TitleView as well :

<NavigationBar title="Color">
  <LeftBar>
    <BarButton title="Back" onPress={() => {
    }} />
  </LeftBar>
   <TitleView>
    <Text> Title here  </Text>
   <Text>Maybe a Sub Title here  </Text>
    </TitleView>
  <RightBar>
    <BarButton title="Cancel" onPress={() => {
    }} />
  </RightBar>
</NavigationBar>

[It just struck me now, would the <TitleView/> override the title prop?]

The only glaring miss i see is the need for an additional logo component. While I really do not have a use for it in my own project(the logo is prominent in a toolbar-like position only in the splash interface and the rest of its usage is within the main body on a few inner screens ), i see that it can be handy for people in their own implementations that need a logo embedded in a toolbar. Can this be as simple as adding an additional <LeftBar/RightBar> <Image source={...} /> </LeftBar/RightBar> adjacent to any existing<LeftBar/RightBar>`?

A search bar for Android as well would be a big plus for sure but rightly can be pushed into a later release since most greenfield projects might just start with the latest RN and its missing ToolbarAndroid void may need to be filled in on priority.

Please let me know the next steps.

grahammendick commented 5 years ago

I’m glad you like the idea. We should merge both the Left and the Right buttons into a single set of Actions/Menu Items. The Left buttons appearing on top of the Right buttons in the menu. You can see what I mean with the following example. On iOS, the Add button is on the left and the Cancel is on the right, but on Android they would both become menu items. 

<NavigationBar title="Color">
  <LeftBar>
    <BarButton title="Add" onPress={() => {
    }} />
  </LeftBar>

  <RightBar>
    <BarButton title="Cancel" onPress={() => {
    }} />
  </RightBar>
</NavigationBar>

We’ll have special Android props for the navigation button and logo because they don’t have iOS counterparts.

We’ll have a look at what happens when we set a title and TitleView on Android. On iOS, the TitleView takes precedence.

Next Steps

We should get the basic use-case working on Android (and iOS). A NavigationBar component with a single title prop. This will put all the key pieces into place and make sure we’re both happy before we move on to the trickier parts.

<NavigationBar title=”Hello” />

How we go about this is up to you. It all depends on how you like to work.

For example, if you like to work things out first before you start coding then you could write a plan in the comments for me to review. It could include a list of the files you plan to create and amend.

Or if you like to get coding first then you could create the PR straight away and push to it whenever you want me to check it over.

What do you think?

arunmenon1975 commented 5 years ago

We should get the basic use-case working on Android (and iOS). A NavigationBar component with a single title prop. This will put all the key pieces into place and make sure we’re both happy before we move on to the trickier parts.

Thanks for not dropping me at the deep end of the pool :) - this definitely makes me a lot less nervous and sounds like just about the right path to start out with.

As i don't have much/any native code writing experience besides the odd changes here and there, it might be better to have my early outputs vetted. I will create and send out the plan as a a comment by tomorrow or at max day-after[have an out-of-town invite for a birthday celebration tomorrow + need just that bit of brushing up(research) before i commit ].

emerson233 commented 5 years ago

I open a PR for this issue. Hope this helps!

grahammendick commented 5 years ago

@emerson233 It does help, thanks a lot! I’m looking forward to reviewing it! @arunmenon1975 I hope you don't feel bad that @emerson233 got in before you.

emerson233 commented 5 years ago

Sorry for the interruption, but we are trying to migrate our project to use this repo instead of react-native-navigation and we need components like NavigationBar to be added as soon as possible.

arunmenon1975 commented 5 years ago

I have been looking at the existing code for the Tab implementations that were recently added as an inspiration and have come to some sort of draft finalization, at least for the list of files that may be required and some 'start code' snippets. Please note that my understanding could/would be entirely erroneous; im still at an early learning stage and am concurrently binge watching/reading tutorials and other relevant content.

Please see a sort of rough plan for implementation listed below:

List of possible files:

A list of added and/or modified files that could be required.

Since a new platform independent module is being built, all files listed for both the platforms below will be new additions.

Android:

iOS:

First-cut implementation

Specific to implementing just <NavigationBar title=”Hello” />, only a subset of files may be required to get going since there is a clear distinction between almost all connected components. The NavigationBarView will manage all of the inner views, including rendering the mandatory title view from the title prop.

Android:

IoS:

Tentative code:

For the first cut going out, the tentative code that may reside in the files is listed below.

Android:

NavigationBarManager.java

package com.navigation.reactnative;
import android.view.View;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.common.MapBuilder;
import com.facebook.react.uimanager.ThemedReactContext;
import com.facebook.react.uimanager.ViewGroupManager;
import com.facebook.react.uimanager.annotations.ReactProp;
import java.util.Map;
import javax.annotation.Nonnull;

public class NavigationBarManager extends ViewGroupManager<NavigationBarView> {
    @Nonnull
    @Override
    public String getName() {
        return "NVNavigationBar";
    }
    @Nonnull
    @Override
    protected TabBarView createViewInstance(@Nonnull ThemedReactContext reactContext) {
        return new NavigationBarView(reactContext);
    }
    @ReactProp(name = "title")
    public void setTitle(NavigationBarView view, @Nullable Integer title) {
        view.setTitle(title);
    }
    @Override
    public View getChildAt(NavigationBarView parent, int index) {
    }
    @Override
    public void addView(NavigationBarView parent, View child, int index) {
    }
}

NavigationBarView.java

package com.navigation.reactnative;
import android.content.Context;
import android.view.ViewGroup;
import androidx.fragment.app.Fragment;
import com.facebook.react.bridge.ReactContext;
import com.facebook.react.uimanager.events.RCTEventEmitter;
import com.facebook.react.views.imagehelper.ResourceDrawableIdHelper;

public class NavigationBarItemView extends ViewGroup {
    Fragment fragment;
    protected String title;
    public NavigationBarItemView(Context context) {
        super(context);
    }
    void setTitle(String title) {
        title = title;
    }
    @Override
    protected void onLayout(boolean changed, int l, int t, int r, int b) {
    }
    @Override
    public Fragment getFragment() {
        return fragment;
    }
}

TitleViewManager.java TBD

TitleViewView.java TBD

IoS:

NVNavigationBarManager.h

#import <React/RCTViewManager.h>

@interface NVNavigationBarManager : RCTViewManager
@end

NVNavigationBarManager.m

#import "NVNavigationBarManager.h"
#import "NVNavigationBarView.h"

@implementation NVNavigationBarManager
RCT_EXPORT_MODULE()
- (UIView *)view
{
    return [[NVNavigationBarView alloc] init];
}
RCT_EXPORT_VIEW_PROPERTY(title, NSString)
@end

NVTitleViewManager.h TBD

NVTitleViewManager.h TBD

grahammendick commented 5 years ago

@arunmenon1975 Thanks for the plan. I now have two people working on the same PR. Can you let me know if you're happy for @emerson233 to take over the work. If you're not happy then please tell me and we can decide how to proceed.

arunmenon1975 commented 5 years ago

@emerson233 It does help, thanks a lot! I’m looking forward to reviewing it! @arunmenon1975 I hope you don't feel bad that @emerson233 got in before you.

Not at all. The Navigation router needs a navigation bar at the earliest at any cost. My effort will have taken a lot more time

@emerson233 I am looking forward to this being merged in for 2 reasons: 1) my project needs it and 2) i want to see the implementation since this will clear a lot of my 'starter' doubts.

grahammendick commented 5 years ago

@arunmenon1975 That's good to hear. Thanks for being so generous.

arunmenon1975 commented 5 years ago

@arunmenon1975 Thanks for the plan. I now have two people working on the same PR. Can you let me know if you're happy for @emerson233 to take over the work. If you're not happy then please tell me and we can decide how to proceed.

Please go ahead with @emerson233 PR. I am looking forward to it as well. There is an element of time (and quality) to be considered and i still don't have the experience to tackle a feature like this quickly. All new RN installations that also use the Navigation router will now have to counter a broken Twitter sample on Android which is not a happy state to be in.

I am looking forward to the PR merger, and with the start that i have made over the last couple of days, it will be quite an apt learning opportunity to see the implementation.

grahammendick commented 5 years ago

Your plan deserves a review even though we're not progressing with the work. It looks excellent, I've only a few comments:

Hope this helps! Let me know if you have any questions

Jianing93 commented 5 years ago

Thanks for your reply!

grahammendick commented 5 years ago

@Jianing93 Sorry, please ignore that review. That was for @arunmenon1975!

@Jianing93 I will review your work on the PR

arunmenon1975 commented 5 years ago

Your plan deserves a review even though we're not progressing with the work. It looks excellent, I've only a few comments:

  • the NavigationBarView Java class should inherit from Toolbar not ViewGroup. Then you don't need to add a setTitle method because there's already one on the Toolbar
  • Make sure you only add what you really need. It's great that you used the Tab implementation as a guide but that means you've added unnecessary methods like getChildAt, addView and getFragment
  • You don't need to start from scratch with the iOS classes. We can use the existing iOS implementations
  • You've forgot to include the changes on the React side. You'll need to rename NavigationBarIOS to NavigationBar and remove the iOS specific logic

Hope this helps! Let me know if you have any questions

Thanks for the feedback. Especially since this is not (rightly)being taken forward for resolving the issue and my initial/beginner doubts would have remained just that.

My early reading led to the official docs, specifically the AppBar section where the example code seems to extend AppCompatActivity 

public class MyActivity extends AppCompatActivity {
  // ...
}

Re-reading the overview mentioned about appCompat's Toolbar widget and about it being more focussed as an app bar than a more broad ActionBar .

Reg point 4, I did not concentrate on the React-side since i was wholly looking at the native code requirements where i was struggling; the React based changes could be handled. But then i guess ,from a library perspective, every change is equally important, including examples.

Thanks again for the feedback. I intend to involve a little deeper into native code in the future since some level of knowledge will be required for any non-trivial solutions. Perhaps via some mini (2-3 screen) concurrent practice side-projects.