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

Support to change Icon in Dropdown Component #401

Closed
brsanthu opened this issue Aug 16, 2016 · 10 comments
Closed

Support to change Icon in Dropdown Component #401

brsanthu opened this issue Aug 16, 2016 · 10 comments
Labels

Comments

@brsanthu
Copy link
Contributor

The DropDown component docs says there is icon prop but specifying as no effect. Looking at the code it looks like dropdown icon is hardcoded. Changing this to use the icon prop, if specified else default dropdown would be really helpful.

I'm looking to use this component to render More Options menu with three vertical dots.

@levithomason
Copy link
Member

Indeed, it looks like that should be:

-<Icon name={'dropdown'} />
+{createIcon(icon)}

I think it should default to dropdown rather than being hard coded, which was an error.

Would you like to make a PR?

@levithomason
Copy link
Member

levithomason commented Aug 16, 2016

It looks like this needs more thought. There is a Dropdown icon type: http://semantic-ui.com/modules/dropdown.html#icon

In that type, the Dropdown can take on the classes labeled icon button. In which case the button needs an icon name.

It looks like this is what intended by the icon prop currently on the Dropdown. Though, I'd like to hash over some proposed API examples of how we will support both the Dropdown icon, and the Button icon before making updates.

@brsanthu
Copy link
Contributor Author

brsanthu commented Aug 17, 2016

Good point. I'm little confused with all the possible combinations of dropdown and popup in Semantic. Don't get me wrong, it is great and has lots of power but it is a challenge in framework like React, where we try to define an API.

To solve this problem now to be able to customize, may be adding a new prop dropdownIcon with default value of dropdown would solve this problem.

On similar topic (may be this should be a new issue, if so please let me know), is it possible to attach a menu (meaning traditional menu with options, with support for nested options) to any item? If so, then we can just create a icon and attach that menu? May be something like this?

<Menu>
    <Button>
    <Menu.Option.Group>
        <Menu.Option text="New"/>
        <Menu.Option text="Edit"/>
        <Menu.Option text="Delete"/>
        <Menu.Option text="More Options">
            <Menu.Option.Group>
                  <Menu.Option text="Clone"/>
                  <Menu.Option text="Save As"/>
            </Menu.Option.Group>
        </Menu.Option>
    </Menu.Option.Group>
</Menu>

or

<Menu>
    <Icon>
    <Menu.Option.Group>
    ...
    </Menu.Option.Group>
</Menu>

@levithomason
Copy link
Member

levithomason commented Aug 17, 2016

Thanks for the suggestions, I'll try them out when I get some time and report back here. I'm currently working on Forms over at #400.

Preliminary Findings

Here is what I gathered yesterday that helped to clarify some things for me:

  • There is only ever one icon
  • Dropdowns can appear as other components like a Button or a Menu Item
  • The icon is either a Dropdown icon, Button icon, or Menu Item icon but only one at a time

image

image

image

image

image

image

image

image

image

Brainstorming Solutions

Given the above, we may be able to use a single icon prop after all. It would just be applied to as either a Dropdown, Button, or Menu Item icon.

Our offline work going on with Forms may introduce a pattern that is helpful to Dropdowns as well. We may allow either component augmentation or specialized subcomponents in Form.Fields. This will facilitate our shorthand props API for Forms. Here are some of the ideas we are playing with:

<Form.Field control='input' type='password' label='Enter password:' />

<Form.Field control={Checkbox} label='I agree to the terms and conditions' />
// or
<Form.Checkbox label='I agree to the terms and conditions' />
<div class="field">
  <label>Enter password:</label>
  <input type='password' />
</div>

<div class="field">
  <Checkbox label='I agree to the terms and conditions' />
</div>

There is also a need library wide to explicitly set the element type for a component. Example, you may want to render a <Segment /> as an HTML <section />. We are considering a library wide standardized prop such as:

<Segment />
<Segment elementType='section' />
<div class="ui segment"></div>
<section class="ui segment"></section>

Applying it to the Dropdown

All of these ideas are ways of augmenting components. I have not thought this idea through at all, but it seems like we might support something along the lines of:

<Dropdown icon='filter' options={...} />
<Dropdown.Button labeled icon='settings' options={...} />
<Dropdown.Item icon='dropdown' options={...} />

// or

<Dropdown icon='filter' options={...} />
<Button elementType={Dropdown} labeled icon='settings' options={...} />
<Menu.Item elementType={Dropdown} icon='dropdown' options={...} />

The top group Dropdown.Item is already taken, so we'd have to solve the name conflict.

The latter group I think makes more sense. Consider the Button, all the props are regular button props (labeled, icon, etc) then the extra props (options) are passed to the Dropdown. This would result in a SUI dropdown being rendered with all the props/classes of both the Button and Dropdown.

As I write this, I'm thinking it might help users better understand the augmentation if we use the prop name as:

<Button as={Dropdown} labeled icon='settings' options={...} />
<Menu.Item as={Dropdown} icon='dropdown' options={...} />

// or

<Dropdown as={Button} labeled icon='settings' options={...} />
<Dropdown as={Menu.Item} icon='dropdown' options={...} />

@levithomason
Copy link
Member

I'll make a PR to fix the icon prop for now, we'll tackle these other features as a separate issue.

@levithomason
Copy link
Member

See #402 for the fix PR closing this issue. See #403 for continued discussion regarding augmentation and other Dropdown types.

@levithomason
Copy link
Member

Merged and released in v0.34.3, let me know if there are any other issues.

@brsanthu
Copy link
Contributor Author

I just tried and it works great. Thank you for the quick fix.

@levithomason
Copy link
Member

No problem, thanks for the report and suggestions!

@annjawn
Copy link

annjawn commented Jul 31, 2018

It looks like the icon prop has a static hard coded size, is there a way to specify the icon size for Dropdown as we would do for the Icon component?

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

No branches or pull requests

3 participants