fix(Dropdown): use item instead of as={Menu.Item}

[levithomason]

Update

Augmenting the Dropdown with a Menu.Item is no longer supported. Instead, use the item prop when using a Dropdown as a menu item. See the changes in this PR for more:

Before

<Dropdown as={Menu.Item} />

After

<Dropdown item />

Original Post

Per #628 (comment), this PR checks for this._dropdown.blur() before calling it. When augmented, the ref may be a composite component ref instead of an actual DOM node. Otherwise, we sometimes get: Uncaught TypeError: a._dropdown.blur is not a function.

[codecov-io]

Current coverage is 95.84% (diff: 100%)

Merging #659 into master will decrease coverage by 0.04%

@@             master       #659   diff @@
==========================================
  Files           879        879          
  Lines          4889       4889          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           4688       4686     -2   
- Misses          201        203     +2   
  Partials          0          0          

Powered by Codecov. Last update d8045cf...2cab8ba

[levithomason]

Though this stops the error, we still have the bug where clicking an item closes the menu, but then switching tabs and switching back opens the dropdown again. This is because it still has focus.

This PR should be updated to actually blur the composite component use case.

[eXtreme]

@levithomason
BTW. I discovered that it is better to do <Menu.Item as={Dropdown}> instead of <Dropdown as={Menu.Item}> - it does not trigger the error and also generates a correct markup: the first one makes <div> for menu.item with dropdown items as <a>) while the latter makes <a> for menu.item causing nesting of <a> (of dropdown.item) inside <a> of menu.item/dropdown.

[tomitrescak]

@eXtreme would you care to provide a full example? I tried several variants of using menu.item with dropdown but no luck

[eXtreme]

@tomitrescak

<Menu>
    <Menu.Item>
        Foo
    </Menu.Item>
    <Menu.Item as={Dropdown} text="Imma dropdown">
        <Dropdown.Menu>
            <Dropdown.Item>Foo</Dropdown.Item>
            <Dropdown.Item>Bar</Dropdown.Item>
            <Dropdown.Divider />
            <Dropdown.Item>Baz</Dropdown.Item>
        </Dropdown.Menu>
    </Menu.Item>
</Menu>

Also works with custom trigger={<span>bar</span>}

[Chris-R3]

How about impementing a blur() method on Menu.Item?
Or do you have any suggestions/ideas on how to approach this issue @levithomason?

[levithomason]

We'll mostly likely do away with using augmentation for this use case. The Dropdown can play the role of a MenuItem or Button, but augmentation is not the right tool for the job. See here: #642.

[levithomason]

Note #659 (comment), this PR is invalid as it circumvents the error by simply removing the feature. It not longer blurs when augmenting a functional component. It needs to be updated to blur in all cases.

[giacomorebonato]

I am trying to guess what the code should do here...
If I replace

_this._dropdown.onBlur();

with

_this._dropdown.props.onBlur()

It isn't what you expect right?

[levithomason]

No, blur() doesn't exist at all. The reason is that a ref returns a DOM Node. The DOM Node interface has a blur() method, but a React ref does not work for functional components so there is no DOM Node to call blur on. Moreover, a ref on a composite component does not return a DOM Node either, it returns a reference to the underlying component instance.

So, there are 3 possible returns values from the ref this._dropdown, a DOM Node, null, and a function. The current code only handles the DOM Node case. It blindly fails for the null and function cases.

The real hang up here on this is the null case, which is what a stateless functional component will return. We need to blur the Node for the Dropdown to work right, but without a working ref there is no way to do that. So, we're gonna have to do some component wrapping gymnastics to get that to work.

[Maxhodges]

bump any big reason why this can't be merged? I have a production app which is suffering

[eXtreme]

@Maxhodges have you tried my "workaround" from this comment?

[Maxhodges]

ah yes, that worked for me. just nicer if I didn't have to workaround ;)
cheers! @eXtreme

[levithomason]

@Maxhodges refer to the comments I've made above for why this isn't yet merged.

[levithomason]

There is a larger issue here that needs solved. We need a consistent and portable way to deal with refs on stateless functional components. The same solution for this issue will solve #1012.

[raptoria]

@eXtreme tried your workaround..but Menu.Item doesn't take a text attribute anymore, it seems. Any work around so that I can see a bit of text associated with the dropdown?

[eXtreme]

@raptoria How is that not working for you? I've tried changing an example on http://react.semantic-ui.com/collections/menu and it works fine with text property.

From what I understand, it's RSui's augmentation, which makes the "text" property being actually passed to Dropdown component, as you may see from inspecting with react devtools:

"t" is actually the "Dropdown" component, which takes "text" property.

The resulting DOM is almost the same as in original SUI (in RSui it has ui item dropdown classname, while in SUI it is ui dropdown item) but the effect is the same.

Can you provide an example of this workaround not working?

[raptoria]

@eXtreme I should clarify. Your workaround works, it just gives me a TypeScript error. I'm a TypeScript n00b so I'm not really sure how to fix it. I guess the problem has to do with the text attribute not being explicitly defined in MenuItem.

ERROR in ./src/components/FileMenu/FileMenu.tsx
(18,32): error TS2339: Property 'text' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<MenuItemProps, ComponentState>> & { chil...'.

this is the code my React component is returning:

      <Menu inverted className={styles.fileMenu}>
      <Menu.Item as={Dropdown} text='File'>
          <Dropdown.Menu className={styles.dropdownMenu}>
            <Dropdown.Item icon='file outline icon' text='New Order Entry' />
            <Dropdown.Item icon='shopping basket icon' text='New Basket' />
            <Dropdown.Item icon='download icon' text='Load Portfolio' />
            <Dropdown.Item icon='file excel outline icon' text='Export to Excel' />
            <Dropdown.Item icon='sign out icon' text='Quit Application' />
          </Dropdown.Menu>
        </Menu.Item>
        <Menu.Item name='Edit'> Edit &nbsp;<Icon name='caret down icon' /></Menu.Item>
        <Menu.Item name='Tools'> Tools &nbsp;<Icon name='caret down icon' /></Menu.Item>
        <Menu.Item name='Views'> Views &nbsp;<Icon name='caret down icon' /></Menu.Item>
      </Menu>

any ideas on how to get rid of the error?

Thanks!!

[levithomason]

@raptoria, and others who come here looking for a solution, it is most likely that using augmentation (the as prop) will not be the recommended solution for this case. A more appropriate workaround, for now, is to simply add className='item' to the Dropdown so it is formatted properly within the Menu.

<Menu attached='top'>
  <Dropdown className='item' icon='wrench' />
</Menu>

This prevents Uncaught TypeError: a._dropdown.blur is not a function since the Dropdown is not attempting to render as a stateless component but remains a class component.

[raptoria]

@levithomason thanks! it works perfectly now.

[levithomason]

Released in [email protected].