feat: Menu component #485

iyadthayyil · 2018-08-08T15:57:42Z

Motivation

Menu component with auto positioning and RTL support:
https://material.io/design/components/menus.html
fixes: #9

Test plan

callstack-bot · 2018-08-08T16:03:16Z

Hey @iyadthayyil, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

kpsroka

Hi Iyad!
First of all, please excuse me for taking so long to review this PR. This should have happened much earlier. Thank you for your contribution. It's exciting to see that you tackle writing this -- quite important -- component.

However, there are two major issues with your approach:

The position of the menu is not updated when the screen changes orientation:
If the menu is larger than available vertical space, it should become a scrollable view. Instead, it flows outside of the screen:

Also, another minor blocker is that the dropdown menu default placement should be below the element that generates it (see relevant material docs). While this is not so for all of elements (indeed some menus should appear on top of the generating element), it should be the default. I believe that we should support steering of the placement of the menu, relatively to the generating element, but it's good enough for the initial version to stick with the default mentioned above.

I also have several other comments about the code. Please address them, or give more support for the choices that you made.

Thanks again!

kpsroka · 2018-08-29T15:44:28Z

src/components/Menu/AnimatedSurface.js

+ * });
+ * ```
+ */
+class Surface extends React.Component<Props> {


Frankly, I don't see much value in having Surface as a separate component. It's used in just one place, and only adds some styles. Can you tell me what use this component has outside of how it's used in your PR?

It's a quick hack to overcome an animation problem I faced when creating a new animated view from the existing Surface component. This was the original way I did it:

const AnimatedSurface = Animated.createAnimatedComponent(Surface);

But this seems to render very choppy animations, would like to know if there is a way to just use Surface without this animation issue.

@iyadthayyil We can change View to Animated.View here: https://github.com/callstack/react-native-paper/blob/master/src/components/Surface.js#L65

Then we wouldn't need Animated.createAnimatedComponent(Surface)

kpsroka · 2018-08-29T15:45:11Z

src/components/Menu/Menu.js

+};
+
+/**
+ * Menus display a list of choices on temporary surfaces. Their placement varies based on the element that opens them.


Nit: add 'elevated' before 'surfaces'. Also elaborate on the placement -- I believe it only adapts based on the source element's position + actual size of the menu.

Ok will do that.

kpsroka · 2018-08-29T15:48:24Z

src/components/Menu/AnimatedSurface.js

+      <Animated.View
+        {...rest}
+        style={[
+          styles.surface,


Since the only rule that styles.surface has is backgroundColor, it will be overridden by the next line. Please remove it.

I just used the same code from the original Surface component, but will change that, also should probably change that in the original Surface component aswell.

kpsroka · 2018-08-29T16:27:04Z

src/components/Menu/Menu.js

+    this.setState({ buttonWidth: width, buttonHeight: height });
+  };
+
+  _toggle = () => {


Nit: This doesn't really "toggle" anything, it just updates the shown/hidden state, so I suggest renaming it to "_updateVisibility"

kpsroka · 2018-08-30T07:47:24Z

src/components/Menu/Menu.js

+      menuSizeAnimation,
+    } = this.state;
+
+    const menuSize = {


You use this constant only once, and really far away from this declaration. It's a bit confusing why you need this until line 282, especially since you keep using menuSizeAnimation.x and menuSizeAnimation.y. Please avoid defining it and instead inline it's use below.

kpsroka · 2018-08-30T07:50:52Z

src/components/Menu/Menu.js

+    }
+
+    // RTL support
+    const leftOrRight = I18nManager.isRTL ? { right: left } : { left };


You use this constant only once, please inline it where it's used.

kpsroka · 2018-08-30T08:07:57Z

src/components/Menu/MenuItem.js

+};
+
+/**
+ * A component to show list of options inside a Menu.


Nit: I believe this component just shows a single option inside a Menu, no?

kpsroka · 2018-08-30T08:08:47Z

src/components/Menu/MenuItem.js

+   */
+  icon?: IconSource,
+  /**
+   * Whether the `MenuItem` is disabled. A disabled `MenuItem` is greyed out and `onPress` is not called on touch.


Nit: write 'item' instead of `MenuItem`.

kpsroka · 2018-08-30T08:09:39Z

src/components/Menu/MenuItem.js

+    maxWidth: maxWidth - 16,
+  },
+  widthWithIcon: {
+    maxWidth: maxWidth - (8 * 6 + 40),


Can you explain where does "8 * 6 + 40" come from? Possibly define a marginWidth const so that it's clear where does '8' come from.

Ok will clear this up with some variables.

kpsroka · 2018-08-30T08:34:55Z

src/components/Menu/MenuItem.js

+      .string();
+
+    if (disabled) {
+      iconColor = titleColor = color(theme.dark ? white : black)


Please define const disabledColor instead, and use it with the ?: operator to define const titleColor and iconColor.

ferrannp · 2018-09-20T10:55:47Z

@iyadthayyil are you still working on this or you need some help? :)

iyadthayyil · 2018-09-23T20:26:44Z

Ok, sorry guys, I have been busy with uni work, will try to get this ready soon.

ferrannp · 2018-10-04T10:25:43Z

Maybe we can do it step by step and take a look to rotation later and other edge cases. Let us know if you need help @iyadthayyil.

raajnadar · 2018-10-11T08:13:25Z

Any ETA for this component?? This component looks beautiful

thevikas · 2019-01-31T05:57:38Z

knock knock

geminiyellow · 2019-02-05T09:11:51Z

please move forward, we need menu

waquidvp · 2019-02-17T18:56:52Z

Hey guys, I want to help merge this PR. Can I rebase from 'upstream' master, the main reason is so that we can remove AnimatedSurface, the upstream Surface now uses Animated.View meaning that Animated.createAnimatedComponent(Surface) won't be needed anymore. See here for more.

waquidvp · 2019-02-18T08:40:37Z

Hey guys, I have made some changes to follow the new way of doing things compared to when the PR was first created. Part of this was to add the typescript typings here: typings/components/Menu.d.ts, this is my first time doing some typescript so would be cool if someone could have a look and give some feedback.

ferrannp · 2019-02-18T11:37:40Z

Hey @waquidvp thanks for coming back to this. I guess you work together with @iyadthayyil ?

Would you mind adding: Screenshots and code for docs? You can see how to do it from other components.

I am also wondering if we could animate scale instead animating height and width, that would allow use to use useNativeDriver. However we can merge first this and then focus on that.

waquidvp · 2019-02-18T20:47:46Z

There were already some screenshots and some code examples. They just weren't in the right place and so wasn't showing up in the generated docs. I have moved it to the right place and the docs look ok now.

waquidvp · 2019-03-03T21:30:22Z

To solve the status bar issue, should a combination SafeAreaView and StatusBar.currentHeight do, or is there any other we handle it in this library?

satya164 · 2019-03-03T23:17:44Z

To solve the status bar issue, should a combination SafeAreaView and StatusBar.currentHeight do, or is there any other we handle it in this library?

Let's do it for now. We can refactor to extract this logic later.

ferrannp · 2019-03-14T11:24:48Z

@satya164 can we have your approval here for merging it?

geminiyellow · 2019-03-22T02:34:28Z

ohhh! amazing! thanks all.

RichardLindhout · 2019-03-22T19:55:48Z

Great work @iyadthayyil! Thanks everyone!

Zefau · 2019-03-28T00:22:50Z

Can the Menu component be used as a Dropdown for TextInput as shown https://material.io/design/components/menus.html#exposed-dropdown-menu ?

Replacing the anchor attribute with TextInput (and using onFocus instead of onPress) won't work.

mihaidaviddev · 2019-04-03T11:19:16Z

@Zefau I also needed to do something similar and I ended up using focus and blur events. It's working on android simulator & device, but for ios is working just in the simulator, I think the surface height bug it's caused by the ScrollView issue described here Any ideea what might be the problem ? This is how the component looks like

import { Keyboard, ScrollView } from "react-native";
import { Menu, TextInput, Theme } from "react-native-paper";
import PaperMenu from "./PaperMenu";

export interface TextInputAutocompleteProps {
  width: number;
  items: string[];
  value: string;
  theme: Theme;
  label: string;
  onChange: (value: string) => void;
}

interface TextInputAutocompleteState {
  filteredItems: string[];
  value: string;
  showMenu: boolean;
}

export default class TextInputAutocomplete extends Component<
  TextInputAutocompleteProps,
  TextInputAutocompleteState
> {
  state = {
    filteredItems: [] as string[],
    value: "",
    showMenu: false
  };
  componentDidMount = () => this.updateStateFromProps(this.props);

  componentWillReceiveProps = (props: TextInputAutocompleteProps) =>
    this.updateStateFromProps(props);

  updateStateFromProps = (props: TextInputAutocompleteProps) =>
    this.setState({ value: props.value });

  filterItems = value => {
    const filteredItems =
      value.trim() !== ""
        ? this.props.items.filter(
            item =>
              item !== value &&
              item.toUpperCase().startsWith(value.toUpperCase())
          )
        : [];
    const showMenu = filteredItems.length > 0;
    this.setState({
      showMenu,
      filteredItems
    });
  };

  onTextInputFocus = () => this.filterItems(this.state.value);

  onTextInputBlur = () => this.setState({ showMenu: false });

  onMenuDismiss = () => {
    this.setState({ showMenu: false });
    Keyboard.dismiss();
  };

  onItemPress = (item: string) => {
    this.onMenuDismiss();
    this.setState({ value: item });
    this.props.onChange(item);
  };

  onTextInputChange = value => {
    this.setState({ value });
    this.filterItems(value);
  };

  render() {
    const menuItems = this.state.filteredItems.map((item, i) => {
      const onPress = () => this.onItemPress(item);
      return (
        <Menu.Item
          style={{ minWidth: this.props.width, maxWidth: this.props.width }}
          onPress={onPress}
          key={i}
          title={item}
        />
      );
    });
    return (
      <PaperMenu
        style={{ marginTop: 50, width: this.props.width }}
        visible={this.state.showMenu}
        onDismiss={this.onMenuDismiss}
        anchor={
          <TextInput
            label={this.props.label}
            mode="outlined"
            theme={this.props.theme}
            value={this.state.value}
            onChangeText={this.onTextInputChange}
            onFocus={this.onTextInputFocus}
            onBlur={this.onTextInputBlur}
          />
        }
      >
        <ScrollView
          scrollEnabled={true}
          showsVerticalScrollIndicator={true}
          keyboardShouldPersistTaps="always"
          style={{ height: 135 }}
        >
          {menuItems}
        </ScrollView>
      </PaperMenu>
    );
  }
}

The PaperMenu is a fork of Menu component to temporary fix the bug #970

RichardLindhout · 2019-04-03T13:47:46Z

On iOS Statusbar.currentHeight does not work so ending up with a undefined top.

mihaidaviddev · 2019-04-03T14:00:48Z

Yes..I use a default value of 40 for now..so i removed StatusBar completely..the issue I have now is with the height of the menu..not the position

raajnadar · 2019-04-03T14:08:45Z

Please create issue ticket instead of posting here. This component is very new so you will see lot of issues.

satya164 assigned kpsroka Aug 16, 2018

kpsroka reviewed Aug 30, 2018

View reviewed changes

ferrannp assigned iyadthayyil and unassigned kpsroka Aug 31, 2018

satya164 changed the base branch from material-next to master September 2, 2018 18:59

ferrannp added the wip label Sep 20, 2018

iyadthayyil mentioned this pull request Sep 26, 2018

fix: Surface View to Animated.View #557

Merged

satya164 force-pushed the master branch 3 times, most recently from f33cab9 to 295a771 Compare October 8, 2018 01:48

satya164 force-pushed the master branch from 1fed58c to 1cf7ae9 Compare October 11, 2018 12:28

raajnadar approved these changes Oct 12, 2018

View reviewed changes

ferrannp added the high priority label Nov 22, 2018

ferrannp removed the wip label Jan 31, 2019

ferrannp added the help wanted label Feb 5, 2019

This was referenced Feb 12, 2019

Feature Request: Text field drop down menu #603

Closed

Menu and Dropdown #9

Closed

waquidvp force-pushed the feat/menu branch from 474971f to 6e3c611 Compare February 18, 2019 06:14

raajnadar requested a review from Trancever February 18, 2019 08:44

waquidvp and others added 13 commits March 2, 2019 20:10

feat: initial attempt at creating ts typings

e93581b

fix: import order to be in alphabetical order

7107764

fix: moved docs markup to be above the class definition

0d23148

fix: update docs wording for more clarity

cb5459a

docs: more fixes to use the correct wording for clarity

dd97efc

fix: add some constants to make values more clear

7e7d0a8

fix: use consts for disabled colors

52582e8

fix: change prop name from button to anchor

42e79ec

fix: use any instead of mixed in type definition

19bf0ba

fix: use the correct types in the definitions

6c7f932

fix: use simpler turnery operation so 'right' or 'left' won't be empty

c448c7c

docs: added comments for more clarity on origin of constants

5ac0fad

use portal

b7456b9

waquidvp force-pushed the feat/menu branch from a97b3cc to b7456b9 Compare March 2, 2019 21:46

waquidvp added 3 commits March 6, 2019 20:46

fix: add status bar padding to top

1e13446

refactor: remove inner view

6ea16bb

fix: add status bar height for all cases

9adb5d0

Trancever approved these changes Mar 21, 2019

View reviewed changes

Trancever merged commit 0c5371a into callstack:master Mar 21, 2019

callstack locked as too heated and limited conversation to collaborators Apr 3, 2019

feat: Menu component #485

feat: Menu component #485

Conversation

iyadthayyil commented Aug 8, 2018

Motivation

Test plan

callstack-bot commented Aug 8, 2018 • edited

kpsroka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrannp commented Sep 20, 2018

iyadthayyil commented Sep 23, 2018

ferrannp commented Oct 4, 2018

raajnadar commented Oct 11, 2018

thevikas commented Jan 31, 2019

geminiyellow commented Feb 5, 2019

waquidvp commented Feb 17, 2019 • edited

waquidvp commented Feb 18, 2019

ferrannp commented Feb 18, 2019

waquidvp commented Feb 18, 2019

waquidvp commented Mar 3, 2019

satya164 commented Mar 3, 2019

ferrannp commented Mar 14, 2019

geminiyellow commented Mar 22, 2019

RichardLindhout commented Mar 22, 2019

Zefau commented Mar 28, 2019

mihaidaviddev commented Apr 3, 2019 • edited

RichardLindhout commented Apr 3, 2019

mihaidaviddev commented Apr 3, 2019

raajnadar commented Apr 3, 2019

callstack-bot commented Aug 8, 2018 •

edited

waquidvp commented Feb 17, 2019 •

edited

mihaidaviddev commented Apr 3, 2019 •

edited