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.

Sign up for GitHub

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

Jump to bottom

[createReactClass] remove createReactClass from ToolbarAndroid/ToolbarAndroid.android.js #21893

Closed
wants to merge 4 commits into from
Closed

[createReactClass] remove createReactClass from ToolbarAndroid/ToolbarAndroid.android.js #21893

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
467aa08
remove createReactClass
nissy-dev Oct 22, 2018
7d34580
fix smart error
nissy-dev Oct 23, 2018
b664e2f
fix flow error
nissy-dev Oct 26, 2018
70b4441
fix typo
nissy-dev Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
275 changes: 149 additions & 126 deletions Libraries/Components/ToolbarAndroid/ToolbarAndroid.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,22 @@
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow
*/

'use strict';

const DeprecatedColorPropType = require('DeprecatedColorPropType');
const DeprecatedViewPropTypes = require('DeprecatedViewPropTypes');
const Image = require('Image');
const NativeMethodsMixin = require('NativeMethodsMixin');
const PropTypes = require('prop-types');
const React = require('React');
const UIManager = require('UIManager');

const createReactClass = require('create-react-class');
const requireNativeComponent = require('requireNativeComponent');
const resolveAssetSource = require('resolveAssetSource');

const optionalImageSource = PropTypes.oneOfType([
Image.propTypes.source,
// Image.propTypes.source is required but we want it to be optional, so we OR
// it with a nullable propType.
PropTypes.oneOf([]),
]);
import type {SyntheticEvent} from 'CoreEventTypes';
import type {ImageSource} from 'ImageSource';
import type {ColorValue} from 'StyleSheetTypes';
import type {ViewProps} from 'ViewPropTypes';
import type {NativeComponent} from 'ReactNative';

/**
* React component that wraps the Android-only [`Toolbar` widget][0]. A Toolbar can display a logo,
Expand Down Expand Up @@ -63,109 +57,133 @@ const optionalImageSource = PropTypes.oneOfType([
*
* [0]: https://developer.android.com/reference/android/support/v7/widget/Toolbar.html
*/
const ToolbarAndroid = createReactClass({
displayName: 'ToolbarAndroid',
mixins: [NativeMethodsMixin],

propTypes: {
...DeprecatedViewPropTypes,
/**
* Sets possible actions on the toolbar as part of the action menu. These are displayed as icons
* or text on the right side of the widget. If they don't fit they are placed in an 'overflow'
* menu.
*
* This property takes an array of objects, where each object has the following keys:
*
* * `title`: **required**, the title of this action
* * `icon`: the icon for this action, e.g. `require('./some_icon.png')`
* * `show`: when to show this action as an icon or hide it in the overflow menu: `always`,
* `ifRoom` or `never`
* * `showWithText`: boolean, whether to show text alongside the icon or not
*/
actions: PropTypes.arrayOf(
PropTypes.shape({
title: PropTypes.string.isRequired,
icon: optionalImageSource,
show: PropTypes.oneOf(['always', 'ifRoom', 'never']),
showWithText: PropTypes.bool,
}),
),
/**
* Sets the toolbar logo.
*/
logo: optionalImageSource,
/**
* Sets the navigation icon.
*/
navIcon: optionalImageSource,
/**
* Callback that is called when an action is selected. The only argument that is passed to the
* callback is the position of the action in the actions array.
*/
onActionSelected: PropTypes.func,
/**
* Callback called when the icon is selected.
*/
onIconClicked: PropTypes.func,
/**
* Sets the overflow icon.
*/
overflowIcon: optionalImageSource,
/**
* Sets the toolbar subtitle.
*/
subtitle: PropTypes.string,
/**
* Sets the toolbar subtitle color.
*/
subtitleColor: DeprecatedColorPropType,
/**
* Sets the toolbar title.
*/
title: PropTypes.string,
/**
* Sets the toolbar title color.
*/
titleColor: DeprecatedColorPropType,
/**
* Sets the content inset for the toolbar starting edge.
*
* The content inset affects the valid area for Toolbar content other than
* the navigation button and menu. Insets define the minimum margin for
* these components and can be used to effectively align Toolbar content
* along well-known gridlines.
*/
contentInsetStart: PropTypes.number,
/**
* Sets the content inset for the toolbar ending edge.
*
* The content inset affects the valid area for Toolbar content other than
* the navigation button and menu. Insets define the minimum margin for
* these components and can be used to effectively align Toolbar content
* along well-known gridlines.
*/
contentInsetEnd: PropTypes.number,
/**
* Used to set the toolbar direction to RTL.
* In addition to this property you need to add
*
* android:supportsRtl="true"
*
* to your application AndroidManifest.xml and then call
* `setLayoutDirection(LayoutDirection.RTL)` in your MainActivity
* `onCreate` method.
*/
rtl: PropTypes.bool,
/**
* Used to locate this view in end-to-end tests.
*/
testID: PropTypes.string,
},
const NativeToolbar = requireNativeComponent('ToolbarAndroid');

type Action = $ReadOnly<{|
title: string,
icon?: ?ImageSource,
show?: 'always' | 'ifRoom' | 'never',
showWithText?: boolean,
|}>;

type ToolbarAndroidChangeEvent = SyntheticEvent<
$ReadOnly<{|
position: number,
|}>,
>;

type ToolbarAndroidProps = $ReadOnly<{|
...ViewProps,
/**
* or text on the right side of the widget. If they don't fit they are placed in an 'overflow'
* Sets possible actions on the toolbar as part of the action menu. These are displayed as icons
* menu.
*
* This property takes an array of objects, where each object has the following keys:
*
* * `title`: **required**, the title of this action
* * `icon`: the icon for this action, e.g. `require('./some_icon.png')`
* * `show`: when to show this action as an icon or hide it in the overflow menu: `always`,
* `ifRoom` or `never`
* * `showWithText`: boolean, whether to show text alongside the icon or not
*/
actions?: ?Array<Action>,
/**
* Sets the toolbar logo.
*/
logo?: ?ImageSource,
/**
* Sets the navigation icon.
*/
navIcon?: ?ImageSource,
/**
* Callback that is called when an action is selected. The only argument that is passed to the
* callback is the position of the action in the actions array.
*/
onActionSelected?: ?(position: number) => void,
/**
* Callback called when the icon is selected.
*/
onIconClicked?: ?() => void,
/**
* Sets the overflow icon.
*/
overflowIcon?: ?ImageSource,
/**
* Sets the toolbar subtitle.
*/
subtitle?: ?string,
/**
* Sets the toolbar subtitle color.
*/
subtitleColor?: ?ColorValue,
/**
* Sets the toolbar title.
*/
title?: ?string,
/**
* Sets the toolbar title color.
*/
titleColor?: ?ColorValue,
/**
* Sets the content inset for the toolbar starting edge.
*
* The content inset affects the valid area for Toolbar content other than
* the navigation button and menu. Insets define the minimum margin for
* these components and can be used to effectively align Toolbar content
* along well-known gridlines.
*/
contentInsetStart?: ?number,
/**
* Sets the content inset for the toolbar ending edge.
*
* The content inset affects the valid area for Toolbar content other than
* the navigation button and menu. Insets define the minimum margin for
* these components and can be used to effectively align Toolbar content
* along well-known gridlines.
*/
contentInsetEnd?: ?number,
/**
* Used to set the toolbar direction to RTL.
* In addition to this property you need to add
*
* android:supportsRtl="true"
*
* to your application AndroidManifest.xml and then call
* `setLayoutDirection(LayoutDirection.RTL)` in your MainActivity
* `onCreate` method.
*/
rtl?: ?boolean,
/**
* Used to locate this view in end-to-end tests.
*/
testID?: ?string,
|}>;

type Props = $ReadOnly<{|
...ToolbarAndroidProps,
forwardedRef: ?React.Ref<'ToolbarAndroid'>,
|}>;

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

render: function() {
const nativeProps = {
...this.props,
};
render() {
const {
onIconClicked,
onActionSelected,
forwardedRef,
...otherProps
} = this.props;
const nativeProps = {...otherProps};
if (this.props.logo) {
nativeProps.logo = resolveAssetSource(this.props.logo);
}
Expand All @@ -191,22 +209,27 @@ const ToolbarAndroid = createReactClass({
}
nativeActions.push(action);
}
// $FlowFixMe - Cannot assign nativeActions to nativeProps.nativeActions
nativeProps.nativeActions = nativeActions;
}

return <NativeToolbar onSelect={this._onSelect} {...nativeProps} />;
},
return (
<NativeToolbar
onSelect={this._onSelect}
{...nativeProps}
ref={forwardedRef}
/>
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To successfully get rid of NativeMethodMixin, you need to do this:

const ToolbarAndroidWithRef = React.forwardRef((props: Props, ref: React.Ref<typeof NativeToolbar>) => {
  return <ToolbarAndroid {...props} forwardedRef={ref} />
});

And then inside ToolbarAndroid, you need to do this:

type Props = $ReadOnly<{|
  // ...
  forwardedRef: React.Ref<typeof NativeToolbar>,
|}>

class ToolbarAndroid extends React.Component<Props> {
  // ...
  render() {
    const {
      onIconClicked,
      onActionSelected,
      forwardedRef: ref,
      ...nativeProps,
    } = this.props;    
    // ...

    return <NativeToolbar onSelect={this._onSelect} {...nativeProps} ref={ref}/>
  }
}

I think React.forwardRef isn't flow typed yet, so you'll have to find a workaround for that. There are other PRs removing NativeMethodsMixin that accomplish this (e.g: #21885)

Copy link
Contributor

Choose a reason for hiding this comment

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

The bit that needs to be copied for now specifically is the FlowFixMe comment above the forwardref call, to suppress the type errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


_onSelect: function(event) {
const position = event.nativeEvent.position;
if (position === -1) {
this.props.onIconClicked && this.props.onIconClicked();
} else {
this.props.onActionSelected && this.props.onActionSelected(position);
}
// $FlowFixMe - TODO T29156721 `React.forwardRef` is not defined in Flow, yet.
const ToolbarAndroidToExport = React.forwardRef(
(props: ToolbarAndroidProps, forwardedRef: ?React.Ref<'ToolbarAndroid'>) => {
return <ToolbarAndroid {...props} forwardedRef={forwardedRef} />;
},
});

const NativeToolbar = requireNativeComponent('ToolbarAndroid');
);

module.exports = ToolbarAndroid;
module.exports = (ToolbarAndroidToExport: Class<
NativeComponent<ToolbarAndroidProps>,
>);