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.

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

RFC: Dropdown header and divider shorthands #1724

Open
saitonakamura opened this issue May 31, 2017 · 15 comments
Open

RFC: Dropdown header and divider shorthands #1724

saitonakamura opened this issue May 31, 2017 · 15 comments
Milestone

Comments

@saitonakamura
Copy link
Contributor

saitonakamura commented May 31, 2017

https://react.semantic-ui.com/modules/dropdown#dropdown-example-header

I wanna use Dropdown.Header and Dropdown.Divider in search or multiple dropdown, but it's currently not supported. So I suppose we should extend shorthand props to support it. But it can be done in many ways so it's better to consider different options.

Header

I think the header shorthand is pretty straitforward

import React from 'react'
import { Dropdown } from 'semantic-ui-react'

const options = [
  { content: 'Important', header: { icon: 'tags', content: 'Filter by tag' } },
  { content: 'Announcement' },
  { content: 'Discussion' }
]

// <Dropdown.Header icon='tags' content='Filter by tag' />
// <Dropdown.Item>Important</Dropdown.Item>
// <Dropdown.Item>Announcement</Dropdown.Item>
// <Dropdown.Item>Discussion</Dropdown.Item>

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

export default DropdownExampleHeader

Divider

I think it should be implemented by using divider shorthand with position as shorthand value. Possible positions will be 'top' | 'bottom' | 'both'.

import React from 'react'
import { Dropdown } from 'semantic-ui-react'

const options = [
  { content: 'Important' },
  { content: 'Announcement', divider: 'top' },
  { content: 'Discussion' }
]

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

const DropdownExampleDivider = () => (
<Dropdown text='Filter' icon='filter' floating labeled button className='icon' options={options} />
)

export default DropdownExampleDivider
@saitonakamura
Copy link
Contributor Author

Implementation from the top of my head

Before Dropdown.js#L1165

return _.map(options, (opt, i) => DropdownItem.create({
  active: isActive(opt.value),
  onClick: this.handleItemClick,
  selected: selectedIndex === i,
  ...opt,
  // Needed for handling click events on disabled items
  style: { ...opt.style, pointerEvents: 'all' },
}))

After

return _.reduce(options, (accumulator, opt, i) => {
  if (opt.header) {
    accumulator.push(DropdownHeader.create({ ...opt.header }))
  }
  if (opt.divider === 'top' || opt.divider === 'both') {
    accumulator.push(<DropdownDivider />)
  }
  accumulator.push(DropdownItem.create({
    active: isActive(opt.value),
    onClick: this.handleItemClick,
    selected: selectedIndex === i,
    ...opt,
    // Needed for handling click events on disabled items
    style: { ...opt.style, pointerEvents: 'all' },
  }))
  if (opt.divider === 'bottom' || opt.divider === 'both') {
    accumulator.push(<DropdownDivider />)
  }
  return accumulator
}

@levithomason
Copy link
Member

Thank you for starting this RFC. In addition to the linked #889, it is also closely related to #1365 (submenu's and nested dropdown menu's).

I'd propose that we standardize on the component key to define which component the props are for. This would apply to any shorthand. Updating the example Dropdowns you provided above:

Header

const options = [
  { component: Dropdown.Header, icon: 'tags', content: 'Filter by tag' },
  { content: 'Important' },
  { content: 'Announcement' },
  { content: 'Discussion' }
]

// <Dropdown.Header icon='tags' content='Filter by tag' />
// <Dropdown.Item>Important</Dropdown.Item>
// <Dropdown.Item>Announcement</Dropdown.Item>
// <Dropdown.Item>Discussion</Dropdown.Item>

Divider

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>

@saitonakamura
Copy link
Contributor Author

In this case why don't do something like

const options = [
  { component: <Dropdown.Header icon='tags' content='Filter by tag' /> },
  { content: 'Important' },
  { content: 'Announcement' },
  { content: 'Discussion' }
]

?

@saitonakamura
Copy link
Contributor Author

saitonakamura commented Jun 9, 2017

Here's another thing. Let's say we implemented this and updated the examples, like, the first one

const options = [
  { text: 'New' },
  { text: 'Open...', description: 'ctrl + o' },
  { text: 'Save as...', description: 'ctrl + s' },
  { text: 'Rename', description: 'ctrl + r' },
  { text: 'Make a copy' },
  { icon: 'folder', text: 'Move to folder' },
  { icon: 'trash', text: 'Move to trash' },
  { component: <Dropdown.Divider /> },
  { text: 'Download As...' },
  { text: 'Publish To Web' },
  { text: 'E-mail Collaborators' },
]

const DropdownExampleDropdown = () => (
  <Dropdown text='File' options={options} />
)

The problem is now the state is fully managed so when clicking on some option we get all of our options bolded. It's because they has no value, so it's empty value now selected and all options are highlighted. But there shouldn't be any state management in a such use case, because it's not the value we wan't but immediate action on option selection.

image

So it raises two questions

  • Should we update all examples to shortand or just leave such use cases be.
  • Maybe we should get rid off subcomponents API completely?

@levithomason
Copy link
Member

Thanks for the PR, let's move discussions over there!

@ifokeev
Copy link

ifokeev commented Oct 19, 2017

need this feature

@9teen90nine

This comment has been minimized.

@quave

This comment has been minimized.

@levithomason
Copy link
Member

levithomason commented Oct 29, 2017

Please avoid +1 and status comments as they send all subscribers emails and notifications. All work is public and status can be tracked by looking at the open referenced PR, #1757. Additionally, emoji reactions on the original description are more useful as they do not send notifications and they enable issue sorting.

@Semantic-Org Semantic-Org locked and limited conversation to collaborators Oct 29, 2017
@Semantic-Org Semantic-Org unlocked this conversation May 18, 2018
@stale
Copy link

stale bot commented Aug 17, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Aug 17, 2018
@layershifter
Copy link
Member

Finally, we have a decision there, we will go with the kind prop, microsoft/fluent-ui-react#512

@stale stale bot removed the stale label Nov 25, 2018
@stale
Copy link

stale bot commented May 24, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale
Copy link

stale bot commented Dec 8, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@callain
Copy link

callain commented Dec 21, 2020

Hello @layershifter, since your feature #4029 is merged, is it possible to use shorthands for Dropdown options to create Header and Divider instead of item ? If yes, how ? I'm not sure to understand the feature correctly

I am in a case where I need a <Dropdown options={options} search multiple selection /> and separate the options in two sublist.
Here is a codesanbox to understand it better: https://codesandbox.io/s/friendly-noether-3rgf9?file=/src/index.js

If we don't use shorthands we have to recode the search, the selection and manage multiple values manually which is a bit annoying

@DreierF
Copy link

DreierF commented Sep 5, 2021

What seems to do the trick after #4029 is to define headers and dividers as follows within the dropdown options:

const options = [
  {
    key: "header1",
    children: () => <Dropdown.Header content="Header" />,
    disabled: true
  },
  {
    key: "divider1",
    children: () => <Dropdown.Divider />,
    disabled: true
  },
  {
    key: "key1",
    value: "value1",
    text: "value1"
  },
...
];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants