1036643 - Refactor dropdown menu as a reusable component

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect)

Description

[Andrei Oprea [mention me on [e-mail]]]

Refactor dropdown as a reusable agnostic component, eg. <DropdownMenu options={options} onChange={handler}/>

Comment 1

[Andrei Oprea [mention me on [e-mail]]]

I've tried out this idea on a dummy menu just to see if I have the "React mindset". The code is here [0] will go forward with the implementation for Loop once someone looks over my code

[0] http://pastebin.mozilla.org/5539475

Comment 2

[Mark Banner (:standard8)]

Can someone remind me where this menu was intended to be shared to? If it is the conversation window - with the cheverons on incoming call, I'm not quite sure I see that it could be shared in a nice way, as there's different resulting functionality - the cheverons are more a button than a menu.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

My take http://jsbin.com/dixujuve/1/edit

Comment 4

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Nicolas Perriault (:NiKo`) from comment #3)
> My take http://jsbin.com/dixujuve/1/edit

I've started from your example and made it look like the component.

http://jsbin.com/dixujuve/13/edit

But I ran into an issue. In the mockups the button you click to open the menu has the bullet (colored icon) on the right, and in the menu it is on the left. So you can't actually use the same element (or you can if you somehow modify it). I haven't been able to figure out a solution to this that would still keep the dropdown menu small and reusable.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> In the mockups the button you click to open the menu has the bullet (colored icon) on the right, and in the menu it is on the left

How about passing a css class name to ease styling the selected item vs. listed ones?

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Here's a quickly drafted PoC http://jsbin.com/dixujuve/19/edit

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Another strategy if we want full semantic flexibility would be to pass an entry content as a prop, though I don't see why that would be needed for now; let's stick with using CSS as much as possible :)

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Refactor Loop dropdown menu into a reusable component

Comment 9

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Refactor Loop dropdown menu into a reusable component

Comment 10

[Andrei Oprea [mention me on [e-mail]]]

Nikò any chance you could get on this on Thursday ?

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8455439 [details] [diff] [review]
Refactor Loop dropdown menu into a reusable component

Review of attachment 8455439 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting approach, though I feel like we could use a more simple approach using a list of object to describe options, a bit like what I suggested in http://jsbin.com/dixujuve/19/edit. Though this obviously can work as well, so we could probably go with it for now and see how it scales on the long term.

I'm concerned with the lack of testing, you'll probably want to cover more of the API and use cases.

I'd like to hear more of :dmose's thoughts on this patch, as I don't feel confident enough to put an r+ with comments to be addressed just yet.

::: browser/components/loop/content/js/panel.jsx
@@ +21,5 @@
>     */
>    var router;
>  
>    /**
> +   *  Reusable dropdown component

"Reusable dropdown menu item component" might be more accurate.

@@ +23,5 @@
>  
>    /**
> +   *  Reusable dropdown component
> +   *  Usage:
> +   *  @type {function} callback The function to be called when the menu item

We've set a format for props description here: https://github.com/mozilla/gecko-dev/blob/0dcae314a72dc1bb195ef27ccd7bb01408daa48a/browser/components/loop/content/shared/js/views.jsx#L94-L102

Please reuse the same format to keep consistent :)

@@ +33,5 @@
> +   *
> +   *  <Menu select={callback} [selected] [className] [menuDropdownTrigger]>
> +   *    <YourMenuItemComponent />
> +   *    <YourMenuItemComponent />
> +   *  </Menu>

Nit: the example code should sit before properties description.

@@ +43,5 @@
> +
> +    render: function() {
> +      var liStyleReset = {
> +        listStyle: "none"
> +      };

I'm concerned we're dealing with default styling reset in components; imho this should live in CSS land.

@@ +52,5 @@
> +      );
> +    }
> +  });
> +
> +  var Menu = React.createClass({

Missing jsdoc. Actually, the one for MenuItem should better live here imho.

Also, declaring React.PropTypes would help ensuring required props are passed.

@@ +60,5 @@
> +        selected: this.props.selected || 0
> +      };
> +    },
> +
> +    open: function(event) {

`event` is unused.

@@ +64,5 @@
> +    open: function(event) {
> +      this.setState({open: true});
> +    },
> +
> +    select: function(option) {

Please document the expected type for the option arg here.

@@ +70,5 @@
> +      this.props.select(this.props.children[option].getDOMNode());
> +    },
> +
> +    render: function() {
> +      if (!this.state.open) {

Nit: using 

if (this.state.open) {
  return;
}

might prevent indenting too much. Matter of taste.

@@ +74,5 @@
> +      if (!this.state.open) {
> +        if (!this.props.menuDropdownTrigger) {
> +          var selectedMenuItem = this.props.children[this.state.selected];
> +          return (
> +            <MenuItem toggleMenu={this.open}>

Nite: I was confused between this.state.open and this.open; renaming to this.openMenu is probably a good idea.

@@ +84,5 @@
> +            <MenuItem toggleMenu={this.open}>
> +              {this.props.menuDropdownTrigger}
> +            </MenuItem>
> +          );
> +        }

This whole if/else block could easily move to some "private" function, eg. this._getMenuItem.

@@ +92,5 @@
> +                                        function(child, index) {
> +          return <MenuItem key={index} toggleMenu={this.select}>
> +                   {child}
> +                 </MenuItem>;
> +      }, this);

Strange identation; How about

var children = React.Children.map(this.props.children, function(child, index) {
  return (
    <MenuItem key={index} toggleMenu={this.select}>
      {child}
    </MenuItem>
  );
}, this);

@@ +99,5 @@
> +      // CSS properties that affect the position of the menu
> +      var ulStyleReset = {
> +        padding: 0,
> +        margin: 0
> +      };

Same remark as above with resetting style within components.

@@ +102,5 @@
> +        margin: 0
> +      };
> +
> +      return (
> +        <ul style={ulStyleReset} className={this.props.menuCSSClass}>

The menuCSSClass props must be documented. Also have a look at transferToProps to propagate attributes to child components http://facebook.github.io/react/docs/reusable-components.html#transferring-props-a-shortcut

@@ +114,4 @@
>     * Availability drop down menu subview.
>     */
>    var AvailabilityDropdown = React.createClass({
> +    changeAvailability: function(DOMElement) {

An instance arg var name shouln't be uppercase; domElement is probably just fine, but please add jsdoc to describe it's a DOMElement.

@@ +120,5 @@
> +        navigator.mozLoop.doNotDisturb = false;
> +      }
> +      if (DOMElement.classList.contains("status-make-dnd")) {
> +        navigator.mozLoop.doNotDisturb = true;
> +      }

I'd expect we set the boolean using a single condition. Also, what if neither class are contained in the DOMElement?

@@ +127,5 @@
>  
>      render: function() {
> +      // XXX choose which menu item is selected by default
> +      // there is room for improvement especially when > 2 items
> +      var defaultItem = navigator.mozLoop.doNotDisturb ? 0 : 1;

I'd expect the default selected item to be either the one passed as the selected one or the first in the list, a bit like an html <select>.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +180,5 @@
> +                                    .querySelector(".status-make-dnd");
> +        TestUtils.Simulate.click(menuTriggerButton);
> +
> +        var menuDOMElem = view.getDOMNode()
> +                                    .querySelector("ul");

This could fit on a single line.

@@ +184,5 @@
> +                                    .querySelector("ul");
> +
> +        // will fail the test if the element does not exist
> +        TestUtils.Simulate.click(menuDOMElem);
> +      });

I would expect a test to ensure the items are properly listed, in the right order. Actually I'd expect more tests at all ;)

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

There's turning out to be enough complexity to this that using it as a shortcut to getting the chevron menus in before uplift seems impractical.  So I'll have a look at his on Monday.

Comment 13

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Side note, this blog post has good tips on how to deal with dynamic children and might be helpful if it's still what we want to do: http://www.mattzabriskie.com/blog/react-referencing-dynamic-children

TL;DR: using refs instead of actual DOM nodes seems cleaner (http://facebook.github.io/react/docs/more-about-refs.html), and cloneWithProps might help http://facebook.github.io/react/docs/clone-with-props.html

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8455439 [details] [diff] [review]
Refactor Loop dropdown menu into a reusable component

Review of attachment 8455439 [details] [diff] [review]:
-----------------------------------------------------------------

Marking this as r- for now to avoid being constantly reminded for overdue requests by bugmail.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8455439 [details] [diff] [review]
Refactor Loop dropdown menu into a reusable component

After chatting with Andrei about this last week, we agreed that this patch isn't worth going forward with.