feat(Dropdown): component shorthand in options prop #1757

saitonakamura · 2017-06-09T16:18:28Z

WIP

This pull request introduces a possibilty to set component shorthand in options Dropdown shorthand. Use cases is Header and Divider use while getting benefits of full state management.

Example

const options = [
  { content: 'Important' },
  { component: <Dropdown.Divider /> },
  { content: 'Announcement' },
  { content: 'Discussion' }
]

// <Dropdown.Item>Important</Dropdown.Item>
// <Dropdown.Divider />
// <Dropdown.Item>Announcement</Dropdown.Item>
// <Dropdown.Item>Discussion</Dropdown.Item>

const DropdownExampleDivider = () => (
  <Dropdown options={options} />
)

Please note that that API is a subject to change (Discussion is in #1724 ), but I took the risk to start implementing it.

Decide how the API should look like
Implement it
Update examles
Update tests

levithomason · 2017-06-09T18:12:51Z

`component`

Regarding the API: { component: <Dropdown.Divider /> }
Let's go with: { component: Dropdown.Divider }

The primary reason is that Dropdown.Divider is a React component (string, class, or function) while <Dropdown.Divider /> is actually a React element. The JSX is compiled to React.createElement(Dropdown.Divider), which, as the name suggests, creates a new element from the provided component.

The secondary reason is that the objects in the array are really props objects. They are spread on the element when created. Currently, all objects are Dropdown.Item props objects. By using a component rather than an element, it allows us to still spread the object as props onto the provided component.

The final reason is that it saves a lot of renders. If an element is passed, it is instantly rendered inline (<Dropdown.Header /> renders an element) regardless of whether or not the Dropdown menu is ever opened. However, by passing the component and its props, we lazily render the items only when needed.

`options`

Since we're now adding much more to the array, these are no longer simply Dropdown (or Select) options. We are, in fact, beginning the shorthand for any the Dropdown.Menu item 🎉

In SUI nomenclature, these are the items within in the menu. I'm going back and forth with hardline consistency and ease of use here.

Technically, this should be added as a menu prop which takes a props object for the Dropdown.Menu. It would contain an items array in that case. I'm leaning this way for consistency and predictability, but it is a little obtuse:

const menu = {
  items: [
    { ... }
  ]
}

<Dropdown menu={menu} />

I like this as it allows the DropdownMenu to have its own active prop and manage its own className buildup. Currently, the Dropdown parent is doing all this. It should also make animations easier once we get there.

levithomason · 2017-06-12T16:47:44Z

Hm, this looks like it will have direct conflicts also with #1038. That PR is getting stale, though, I recall changing the options to shorthand introduces a cascade of side effects that have to be dealt with.

This ends up affecting all rendering, filtering, and callbacks. Let me circle back over there and see where this lands us.

Also, if you have time, go ahead and take a look at it yourself and get up to speed on where it was left off.

layershifter · 2017-07-25T08:01:16Z

`component`

@levithomason I think that we should go with component as string or event type:

[
{ component: Dropdown.Divider, as: Divider },
{ component: 'divider', as: Divider },
{ type: 'divider', as: Divider },
]

It will allow to make render logic more simple:

const { type } = item
const factories = {
  divider: Dropdown.Divider,
  item: Dropdown.Item,
  menu: Dropdown.Menu,
}
const factory = factories[type] || Dropdown.Item

return factory(item)

codecov-io · 2017-07-26T19:14:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4f29636). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1757   +/-   ##
=========================================
  Coverage          ?   99.75%           
=========================================
  Files             ?      145           
  Lines             ?     2488           
  Branches          ?        0           
=========================================
  Hits              ?     2482           
  Misses            ?        6           
  Partials          ?        0

Impacted Files	Coverage Δ
src/modules/Dropdown/Dropdown.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f29636...1604a5f. Read the comment docs.

saitonakamura · 2017-07-26T19:27:55Z

Sorry for my absence, i'm back and eager to continue my work

Let's go with: { component: Dropdown.Divider }

Perfectly sane, I forgot about renders and stuff

Technically, this should be added as a menu prop which takes a props object for the Dropdown.Menu

In that case it will be options shorthand for ease of use and backwards compatibility and new menu shorthand for complex stuff, right?

I think that we should go with component as string or event type:

I think it can be error-prone, cause linters won't complain in case of typo, but not with Dropdown.Divider from import

saitonakamura · 2017-07-26T19:59:51Z

I've updated the code to { component: Dropdown.Divider } and menu version
@levithomason I currently don't understand will the options prop be appliable for component shorthand or not, can you clarify it? Cause at first I was thinking that it won't be, but I can't see the way to separate getMenuOptions logic with options and with menu. The main problem is that we should keep the order of headers, dividers and items. Well, except just copypasting a bunch of code, and I don't fancy that notion.

levithomason · 2017-07-29T18:47:04Z

I've marked this PR for review and will take a look as soon as I can. Would really love to get this one merged. Thanks for persisting!

ssageghi · 2017-08-02T09:19:22Z

Is merge Complete?
can we use it?

saitonakamura · 2017-08-17T11:21:37Z

Hi, guys. Are we agreed on the api? I can start implementing tests and updating documentation if we are and refactor code later.

levithomason · 2017-08-18T04:21:02Z

@levithomason I think that we should go with component as string or event type:

@layershifter IMHO, the as prop should be reserved for rendering the Divider as something else. I'd like to go with the currently implemented solution where component is a real component.

In general, I prefer code over configuration as it is more flexible. Consider this:

{ component: MyHOC(Dropdown.Divider) }

This cannot be done with a string based configuration.

levithomason · 2017-08-18T04:24:19Z

docs/app/Examples/modules/Dropdown/Content/DropdownExampleDescription.js

-      <Dropdown.Item description='5 new' text='Discussion' />
-    </Dropdown.Menu>
-  </Dropdown>
+  <Dropdown text='Filter Tags' floating labeled button icon='filter' className='icon' options={options} />


I think instead of overloading the options here, we use a new items prop. There is quite a bit of logic based on the options for keyboard nav and selected/active/disabled states. Options is also meant to reflect the idea of select options.

My thinking with items is that is reflects any valid Menu item, including headers and dividers. This will also play well with <Menu items={} /> and <Dropdown.Menu items={} />.

Thoughts?

levithomason · 2017-08-18T04:27:43Z

src/modules/Dropdown/Dropdown.js

@@ -1147,6 +1155,15 @@ export default class Dropdown extends Component {
    })
  }

+  renderComponent = (component, props) => {
+    switch (component) {


Rather than a method for this, we should just invoke React.createElement() via JSX syntax. This will allow the user to pass any class or functional component. They may pass an entirely different component than Dropdown.Divider, or one of these wrapped in a HOC which would fail the switch cases.

-this.renderComponent(component, { ...componentProps, key: i }) +<component {...componentProps} key={i} />

I think the renderComponent method then goes away completely.

levithomason · 2017-08-18T04:30:52Z

src/modules/Dropdown/Dropdown.js

@@ -768,6 +773,9 @@ export default class Dropdown extends Component {

    // filter by search query
    if (search && searchQuery) {
+      // filter any component elements like Header or Divider
+      filteredOptions = _.filter(filteredOptions, opt => !opt.component)


Oh, this is pretty simple. Perhaps we should just keep options as the prop for this case. For some reason, I hadn't thought of this simple fix. I believe this will work in all cases.

levithomason · 2017-08-18T04:31:39Z

docs/app/Examples/modules/Dropdown/Content/DropdownExampleHeader.js

+}
+
+const DropdownExampleHeader = () =>
+  <Dropdown text='Filter' icon='filter' floating labeled button className='icon' menu={menu} />


I like this API the most. This is the most consistent with where we are headed with shorthand and subcomponent alignment.

levithomason · 2017-08-18T04:51:44Z

will the options prop be applicable for component shorthand or not, can you clarify it?

I'm a little torn on this myself, however, I think we need to abandon the options prop. We have the <Select /> addon component which takes options. This makes sense because of the parity with an HTML select and option children. This is a convenience wrapper around the Dropdown.

The Dropdown, however, has a menu child. I think then we should keep parity between subcomponents and shorthand and go with a menu prop only for the Dropdown:

Final API propsal:

<Select options={} />   // as it is today, no change
<Dropdown menu={} />    // Takes Dropdown.Menu shorthand

It makes sense for a Dropdown.Menu to pass literal shorthand values to its items prop:

DropdownMenu.js

DropdownMenu.create = createShorthandFactory(DropdownMenu, items => ({ items })

Dropdown.js

// When rendering the menu
DropdownMenu.create(this.props.menu)

This means arrays will be passed to the items prop:

// Array of DropdownItem primitive values
<Dropdown menu={['First', 1, 2, 3, 'Last]} />

// Array of DropdownItem props objects 
<Dropdown menu={[{ text: 'One', value: 1 }]} />

// Array of elements
<Dropdown menu={[<Dropdown.Item />, <OtherItem />]} />

Whilst props objects and elements can still be used.

// DropdownMenu props
<Dropdown menu={{ items: [ ... ], visible: true }} />

// An element
<Dropdown menu={<Dropdown.Menu />} />
<Dropdown menu={<AnotherMenu />} />

Note, the createShorthandFactory() method handles all this already, I'm just noting the usage we'd get out of it ;)

ifokeev · 2017-10-19T16:16:57Z

up

dennysem · 2017-10-19T16:18:02Z

+1

…ponent-option

saitonakamura · 2017-10-21T17:47:16Z

@levithomason DropdownMenu.create(this.props.menu) in this case we move a lot of logic from Dropdown to DropdownMenu, right?
Methods like renderOptions, getMenuOptions, search and keyboard navigation goes into DropdownMenu?

levithomason · 2017-10-22T15:31:36Z

Yes, sir. Right now, the Dropdown is assuming all the functionality of the Menu as it creates and manages both.

saitonakamura · 2017-10-24T15:49:42Z

I started refactoring it and stumbled into a problem, for example: we have method getSelectedItem which seem as responsibility of Menu cause you unable to get the possible options from Dropdown, but it's used in method renderText.

I can see a solution, but it seems very hacky: we can get selected item by subscribing by some callback, like onItemSelected and save it to Dropdown state

levithomason · 2017-10-29T15:57:29Z

we can get selected item by subscribing by some callback, like onItemSelected and save it to Dropdown state

This sounds like the idiomatic React approach. Let's go with this for now.

saitonakamura · 2018-04-02T08:00:11Z

@levithomason as you can understand, I can't proceed with this, unfortunately (I left the project in which this feature was needed and I don't have enough willpower). Despite it's obvious, I'm still really sorry for it, guys. Let's close this PR.

levithomason · 2018-04-11T14:31:52Z

Understood, it happens :)

feat(Dropdown): component shorthand in options prop

1c0ff09

This was referenced Jul 21, 2017

Dropdown suggestions: controlled children #1885

Closed

Dropdown: Error occurred with keyboard action #1875

Closed

Merge branch 'master' into feat/dropdown-component-option

7405ded

new api for component shorthand

936459f

menu shorthand

1ca8012

levithomason added the ready for review label Jul 29, 2017

layershifter mentioned this pull request Aug 2, 2017

DropDown #1920

Closed

levithomason mentioned this pull request Aug 3, 2017

fix(Dropdown): your description #1927

Closed

levithomason reviewed Aug 18, 2017

View reviewed changes

This was referenced Sep 17, 2017

Dropdown: ignores closeOnChange if it has children and isn't a search #2041

Closed

fix(Dropdown): keep dropdown open when closeOnChange is set to false #2074

Closed

Merge remote-tracking branch 'upstream/master' into feat/dropdown-com…

1604a5f

…ponent-option

levithomason mentioned this pull request Oct 29, 2017

RFC: Dropdown header and divider shorthands #1724

Open

saitonakamura closed this Apr 11, 2018

levithomason mentioned this pull request May 18, 2018

feat(Dropdown): add managed state support for children #2807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Dropdown): component shorthand in options prop #1757

feat(Dropdown): component shorthand in options prop #1757

saitonakamura commented Jun 9, 2017

levithomason commented Jun 9, 2017 •

edited

Loading

levithomason commented Jun 12, 2017

layershifter commented Jul 25, 2017 •

edited

Loading

codecov-io commented Jul 26, 2017 •

edited

Loading

saitonakamura commented Jul 26, 2017 •

edited

Loading

saitonakamura commented Jul 26, 2017 •

edited

Loading

levithomason commented Jul 29, 2017

ssageghi commented Aug 2, 2017

saitonakamura commented Aug 17, 2017

levithomason commented Aug 18, 2017

levithomason Aug 18, 2017

levithomason Aug 18, 2017 •

edited

Loading

levithomason Aug 18, 2017

levithomason Aug 18, 2017

levithomason commented Aug 18, 2017 •

edited

Loading

ifokeev commented Oct 19, 2017

dennysem commented Oct 19, 2017

saitonakamura commented Oct 21, 2017

levithomason commented Oct 22, 2017

saitonakamura commented Oct 24, 2017

levithomason commented Oct 29, 2017

saitonakamura commented Apr 2, 2018

levithomason commented Apr 11, 2018

feat(Dropdown): component shorthand in options prop #1757

feat(Dropdown): component shorthand in options prop #1757

Conversation

saitonakamura commented Jun 9, 2017

levithomason commented Jun 9, 2017 • edited Loading

component

options

levithomason commented Jun 12, 2017

layershifter commented Jul 25, 2017 • edited Loading

component

codecov-io commented Jul 26, 2017 • edited Loading

Codecov Report

saitonakamura commented Jul 26, 2017 • edited Loading

saitonakamura commented Jul 26, 2017 • edited Loading

levithomason commented Jul 29, 2017

ssageghi commented Aug 2, 2017

saitonakamura commented Aug 17, 2017

levithomason commented Aug 18, 2017

levithomason Aug 18, 2017

Choose a reason for hiding this comment

levithomason Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

levithomason Aug 18, 2017

Choose a reason for hiding this comment

levithomason Aug 18, 2017

Choose a reason for hiding this comment

levithomason commented Aug 18, 2017 • edited Loading

ifokeev commented Oct 19, 2017

dennysem commented Oct 19, 2017

saitonakamura commented Oct 21, 2017

levithomason commented Oct 22, 2017

saitonakamura commented Oct 24, 2017

levithomason commented Oct 29, 2017

saitonakamura commented Apr 2, 2018

levithomason commented Apr 11, 2018

levithomason commented Jun 9, 2017 •

edited

Loading

`component`

`options`

layershifter commented Jul 25, 2017 •

edited

Loading

`component`

codecov-io commented Jul 26, 2017 •

edited

Loading

saitonakamura commented Jul 26, 2017 •

edited

Loading

saitonakamura commented Jul 26, 2017 •

edited

Loading

levithomason Aug 18, 2017 •

edited

Loading

levithomason commented Aug 18, 2017 •

edited

Loading