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

Curious behaviour with DropdownToggle #98

Closed
arcanis opened this issue Aug 15, 2016 · 1 comment · Fixed by #124
Closed

Curious behaviour with DropdownToggle #98

arcanis opened this issue Aug 15, 2016 · 1 comment · Fixed by #124
Labels

Comments

@arcanis
Copy link

arcanis commented Aug 15, 2016

It seems that there's a particular behaviour that tells the DropdownToggle component (and maybe others) to use the children as "master button" if there's a single one, instead of creating a new one. I feel like it's a mistake:

  • It's not an intuitive behaviour - without looking at the source code, one might wonder what happens (I sure did)

  • The tag prop already allows us to customize the way the button is rendered, and is much more idomatic than adding props to already-rendered components.

  • It makes it quite harder to use icons inside a DropdownToggle (because the icon props are rewritten, so the classNames are lost). For example, the following doesn't work:

    <DropdownToggle>
        <i className={`fa fa-user`} />
    </DropdownToggle>

    One has to write the following:

    <DropdownToggle>
        <i className={`fa fa-user`} /> <span></span>
    </DropdownToggle>

    In order to bypass the check.

  • Additional issues may arise (for example, without testing, I guess that such a pattern also breaks refs, and triggers more renders than required).

Would you consider removing this behaviour?

@eddywashere
Copy link
Member

That is a bit weird. I am 👍 for removing that. I think at first I wanted a generic component for copying props onto whatever children are passed but it's really a dropdown button that behaves similar to other components (tag can be overriden). I don't think this pattern is followed anywhere else. Thanks for calling this out. I'll leave this open until this is resolved.

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

Successfully merging a pull request may close this issue.

2 participants