DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite #1096

mtscout6 · 2015-07-31T21:09:17Z

Major part of #526

Adds:

Keyboard Navigation
Aria properties for Assistive Technology
Option to use custom menu with any of the dropdown variations
NavDropdown component (Similar to DropdownButton but specifically for use within Nav components.
DropdownToggle Component which can be used as an alternative to the DropdownButton title prop. Can either be directly imported or used as a static property of the DropdownButton with DropdownButton.Toggle. (Useful should you want to use glyphicons or custom html within the toggle that's not a simple string.)
SplitToggle Similar to the DropdownToggle but targeted at the SplitButton's toggle.

Changed:

Event handling of all these components to be in line with Change calls to e.preventDefault() to allow onClick and onSelect calls to override #419
Written with ES6 class syntax

Removed:

DropdownStateMixin - Everything is using ES6 class syntax so no more mixin usage

Work Remaining: (If anyone would like to help here I'd greatly appreciate it)

Update all doc examples
Implement all test stubs
Add generic doc section for onSelect handlers per Change calls to e.preventDefault() to allow onClick and onSelect calls to override #419

Potential Problems that may still exist:

Nav component onSelect handler is passed to NavDropdown the signatures don't match so we will either need to switch over Nav, NavBar and NavItem to the new signature or add some kind of temporary adaptor till we can address them. I'm not sure which option will involve the least amount of work.

Minor inconveniences to figure out: (Should not prohibit this PR)

PropType validation with objectRestSpread usage. (Part of BootstrapMixin solution for ES6 Classes #646)

I'm hoping that that's everything needed to put a bow tie on this.

jquense · 2015-07-31T22:16:44Z

woohoo! gonna dig into this a bit tomorrow, I have a few cases at work where I can try it out.

mtscout6 · 2015-08-01T15:02:52Z

I'm not sure which option will involve the least amount of work.
-mtscout6 (myself earlier)

Digging deeper this may be complicated pending the outcome of #419 (comment)

taion · 2015-08-02T04:31:24Z

👏

jquense · 2015-08-05T03:40:13Z

src/DropdownMenu.js

+  }
+
+  handleDocumentClick(event) {
+    let inTree = domUtils.isNodeInTree(event.target, React.findDOMNode(this));


I think this is probably now a duplicate of domUtils.contains which was added recently-ish

jquense · 2015-08-05T03:55:41Z

gonna try and help out with some docs tomorrow if I get some time :)

jquense · 2015-08-08T17:22:54Z

Do we have further consensus on whether we are pushing forward with the onSelect event, or going to try a different approach?

jquense · 2015-08-10T15:15:45Z

src/DropdownMenu.js

+    }
+  }
+
+  bindRootCloseHandler() {


It would be great to just use RootCloseWrapper here, but I don't think it'll work as it adds a wrapping div, @taion any thoughts?

w/r/t RootCloseWrapper, I originally wrote it to use a wrapping div because I couldn't be sure that whatever custom popover I wrapped it with wouldn't do something dumb with onClick.

For something like this where we control what gets rendered (i.e. with the outer-most ul), I'd modify RootCloseWrapper to have a separate path that just injects the onClick prop instead of using the wrapping div.

Thats sort of the rub here as well, folks can now provide custom menu components, so we don't know for certain that the component will handle it...

#render() here always renders a ul as the outer-most DOM node, and we control it directly, no? Unless I'm misreading this code. i.e. we can just inject onClick into that ul from RootCloseWrapper painlessly.

bah sorry yes. I was conflating this question along with another one about whether it makes sense to push the root click to the DropdownBase instead of the menu, so all menus' benefit from it.

Oh sorry, I've barely gotten started reading through the rest of the PR.

How much does the rendering break if we add the wrapping div?

mtscout6 · 2015-08-10T15:58:31Z

@jquense I like your earlier proposal for the events, it dramatically simplifies things.

taion · 2015-08-11T00:07:36Z

src/DropdownBase.js

+const TOGGLE_REF = 'toggle-btn';
+
+export default class DropdownBase extends React.Component {
+  constructor(props, Toggle=DropdownToggle) {


I believe the 2nd param to React.Component#constructor is supposed to be context.

Could we just use this.constructor.Toggle below? Or set Toggle on the prototype?

On second thought, this.constructor.Toggle will break everyone using IE <= 10 because static inheritance won't work there, so I'd suggest putting Toggle on the prototype, e.g. below do SplitButton.prototype.Toggle = SplitToggle.

taion · 2015-08-11T00:17:05Z

This is super cool.

taion · 2015-08-11T00:22:56Z

src/DropdownBase.js

+      labelledBy: this.props.id
+    };
+
+    if (menu === undefined) {


cc @jquense

This is actually the opposite of what we decided to do for Modal, i.e. the support for custom modal dialogs is going into react-overlays, while the R-B Modal intentionally has fairly limited support (and a different API) for specifying a custom modal dialog class.

It might be nice to be consistent here if possible.

I was thinking about that myself and i am a bit torn about this one. If there is a more generic dropdown implementation that could be refactored out, its probably to early to know what that'd look like yet. On the other hand i think the level of customizability here is probably on par with what you could do with bootstrap proper. We, for instance gut the built in dropdown (in our non react projects), and essentially just use the styles. this api would let me do that in react, which I really appreciate.

Sure - the right answer might just be to make the R-B Modal's customDialogComponent API look a little more like this. I don't think it'd affect anybody deciding between using R-B Modal and react-overlays, but it'd make things line up better between R-B modals and dropdowns.

Ya definitely, as it is, the custom dialog prop is sort of a 'good enough' solution for the time anyway. I'm all for consistency

mtscout6 · 2015-08-12T20:01:14Z

@jquense With that PR complete did you see anything else that's needed before we bring this in? If it's complete I can do a squash and rebase

taion · 2015-08-12T20:03:17Z

@mtscout6 I think we still need to fix the way we pass in Toggle to the constructors, and figure out if we're okay with being inconsistent between here and Modal for how we specify custom menus v custom modal dialogs.

jquense · 2015-08-12T23:30:13Z

So there is a good, point here: #526 (comment) the current PR will break users using a custom menuItem, since the extractors look for not known types and assume they are menu's.

We can always say that these cases need to have a menu wrapped around them, or perhaps create a more public api for telling the dropdown about types.

taion · 2015-08-12T23:36:48Z

@jquense An alternative might be to use something like dialogComponent (maybe menuComponent?) per Modal to specify the custom menu class.

jquense · 2015-08-12T23:41:28Z

@taion i would like to keep the current way if possible, it seems the more ergonomic of the two api (to me anyway). One issue with that approach tho is that you'd have to do something similar for the Button and Toggle components as well and that seems ugly...

Ultimately the issue is that the Dropdown really needs to know what things are menu items, or toggles, or menus. The most straight forward way to me is just to duck type it with a static property or something, not super elegant.

taion · 2015-08-12T23:43:04Z

That makes sense. What do you think of doing the same for Modal, then, and moving toward deprecating dialogComponent there?

jquense · 2015-08-12T23:45:27Z

seems reasonable to me, tho that case is a little different, since the user is almost always going to specify a custom dialog, whereas here they may just specify a menu to add some classes or something.

The other thing might be that maybe we can jsut get rid of that feature on the modal completely, and point ppl to react-overlays if they need something more custom...I still need to see what the diff is between that and our implementation tho, before I can say its easy to do.

taion · 2015-08-13T00:02:00Z

To make sure I'm not misunderstanding you, is your proposal that the children of a dropdown should be:

Components that are explicitly labeled with a role (toggle, button, or menu); at most one of each
Some set of components, none of which are explicitly labelled with a role

If the user does (1), then we use the explicitly labeled components. If the user does (2), then everything proceeds per the default.

The alternatively would be something like exposing toggleComponent (is this just the existing title prop?) and dropdownButton props and forcing the explicit use of those.

Thinking a little more, I'm not sure which semantically feels better. I like the elegance of specifying everything under children, but the difference between the custom and non-custom API looks a little magical; I don't really have a better answer, though.

taion · 2015-08-13T00:04:34Z

To add to that, one difference between this and the API for Modal is that the children of the Modal have no special semantic meaning (except for the purposes of injection onHide into the header), and otherwise just behave as a children array.

This is a bit different in that the different children have different semantic meanings; e.g. it looks like Toggle and Menu would be invariant to things like re-orderings, and they won't get passed around as-is.

jquense · 2015-08-13T02:23:15Z

I think maybe some sort of type prop might be a good general API. If we could do something fairly foolproof we could not have to mess with things like linkProps or other awkward child pass throughs. not sure it would work, but it would be cool if it did. I don't like the magic, but elements as props always feels really awkward to me as well

jquense · 2015-08-13T15:35:47Z

examples of what I am thinking of...

<DropdownButton>
  <Button bsRole='toggle' >open</Button>

  <CustomMenu bsRole='menu'>
    <MenuItem>Item 2</MenuItem>
  </CustomMenu>
</DropdownButton>

or

<NavItem><Link bsRole='link'>client route</Link></NavItem>

I want to say that whatever the API is it is really an escape hatch. Most folks shouldn't need this sort of behavior, In general it will be other library authors (like react-router-bootstrap).

In this particular case, I think the "magic" is in assuming certain components are Menus. It seems like the perhaps we should be explicitly checking for menus and toggles, and just assume everything else is a MenuItem, which preserves the current behavior.

essentially a component has two "modes" for dealing with children, if a bsRole is defined on any child, then there is a strict whitelist of components allowed, only children with a role, and only whitelisted roles for that component. Otherwise it just does its default, such as wrapping children in a menu.

jquense · 2015-08-17T20:27:50Z

ok I checked a few things off I think, Are more pending items before we merge this? In liee of general onSelect documentation perhaps we should document controlled/uncontrolled props in getting started or something? If so does that block the PR here?

jquense · 2015-08-20T19:06:51Z

anything else here? LGTM otherwise (baring some commit squashing)...

mtscout6 · 2015-08-20T20:18:46Z

Looks good to me, though I don't have time to do the squashing for this before tomorrow.

taion · 2015-08-20T22:09:09Z

webpack/docs.config.js

@@ -21,6 +21,9 @@ if (options.debug) {
 }

 export default _.extend({}, baseConfig, {
+
+  devtool: options.debug ? 'source-map' : null,


Probably shouldn't change this here either.

taion · 2015-08-20T22:10:36Z

This is really cool BTW. Thanks so much for all the work you've put into this - the new "custom" API is fantastic.

jquense · 2015-08-21T00:30:47Z

Ya it really is super sweet and going to save me a ton of work in the long run, thanks @mtscout6 !

mtscout6 · 2015-08-21T14:01:29Z

I'm going to give the rebase and squash a go right now.

mtscout6 · 2015-08-21T14:07:13Z

Well, that rebase went quicker than I was expecting.

mtscout6 · 2015-08-21T14:10:46Z

😢 Linting killed it, I'll get that fixed. Give me a few minutes.

…ely rewritten Adds: - Keyboard Navigation - Aria properties for Assistive Technology - Option to use custom menu with any of the dropdown variations - `NavDropdown` component (Similar to `DropdownButton` but specifically for use within `Nav` components. - `DropdownToggle` Component which can be used as an alternative to the `DropdownButton` `title` prop. Can either be directly imported or used as a static property of the `DropdownButton` with `DropdownButton.Toggle`. _(Useful should you want to use glyphicons or custom html within the toggle that's not a simple string.)_ - `SplitToggle` Similar to the `DropdownToggle` but targeted at the `SplitButton`'s toggle. - Generic `Dropdown` component for easy dropdown customization. Changed: - Event handling of all these components to be in line with #419 - Written with ES6 class syntax Removed: - DropdownStateMixin - Everything is using ES6 class syntax so no more mixin usage

mtscout6 · 2015-08-21T15:05:59Z

Linting is all fixed and everything is now squashed.

taion · 2015-08-21T15:20:56Z

Well what are we waiting for then? :D

DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite

AlexKVal · 2015-08-21T19:38:10Z

Yay 🎉 Finally 🚀
It was a long way.

AndersDJohnson · 2015-10-11T15:14:50Z

+1 for sub-components instead of title attributes on components like NavDropdown.

Though, this appears to be somewhat achievable already, e.g.:

var title = <span><i className="fa fa-gear"></i> Settings</span>;
var drop = <NavDropdown eventKey={3} title={title}> ... </NavDropdown>;

mtscout6 force-pushed the dropdown-revisited branch from 9f0c4d1 to fca0be7 Compare July 31, 2015 21:10

jquense reviewed Aug 5, 2015
View reviewed changes

AlexKVal mentioned this pull request Aug 7, 2015

BootstrapMixin solution for ES6 Classes #646

Closed

jquense mentioned this pull request Aug 8, 2015

Dropdown docs and tweaks #1125

Merged

jquense reviewed Aug 10, 2015
View reviewed changes

taion added this to the v0.25 Release milestone Aug 10, 2015

taion reviewed Aug 11, 2015
View reviewed changes

This was referenced Aug 11, 2015

Support disabling of DropdownButton. #1129

Closed

Make Modal wrapper configurable #748

Closed

AlexKVal mentioned this pull request Aug 12, 2015

Allow totally custom styles via 'bsStyle' #988

Closed

4 tasks

mtscout6 mentioned this pull request Aug 12, 2015

DropdownButtons need an overhaul. #526

Closed

30 tasks

jquense mentioned this pull request Aug 13, 2015

close button dropdown component #1130

Closed

AlexKVal mentioned this pull request Aug 17, 2015

DropdownButton doesn't close #1170

Closed

taion reviewed Aug 20, 2015
View reviewed changes

mtscout6 force-pushed the dropdown-revisited branch from c22680f to 0e161f4 Compare August 21, 2015 14:06

mtscout6 force-pushed the dropdown-revisited branch from 0e161f4 to 346501e Compare August 21, 2015 15:05

mtscout6 changed the title ~~[WIP] DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite~~ DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite Aug 21, 2015

mtscout6 added a commit that referenced this pull request Aug 21, 2015

Merge pull request #1096 from react-bootstrap/dropdown-revisited

3a9eec9

DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite

mtscout6 merged commit 3a9eec9 into v0.25-rc Aug 21, 2015

mtscout6 deleted the dropdown-revisited branch August 21, 2015 15:22

AlexKVal mentioned this pull request Aug 21, 2015

Use CustomPropTypes from react-prop-types #937

Closed

madisvain mentioned this pull request Aug 29, 2015

React-Bootstrap components don't handle onClick react-bootstrap/react-router-bootstrap#112

Closed

DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite #1096

DropdownButton, SplitButton, DropdownMenu, MenuItem Rewrite #1096

Conversation

mtscout6 commented Jul 31, 2015

jquense commented Jul 31, 2015

mtscout6 commented Aug 1, 2015

taion commented Aug 2, 2015

Choose a reason for hiding this comment

jquense commented Aug 5, 2015

jquense commented Aug 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtscout6 commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtscout6 commented Aug 12, 2015

taion commented Aug 12, 2015

jquense commented Aug 12, 2015

taion commented Aug 12, 2015

jquense commented Aug 12, 2015

taion commented Aug 12, 2015

jquense commented Aug 12, 2015

taion commented Aug 13, 2015

taion commented Aug 13, 2015

jquense commented Aug 13, 2015

jquense commented Aug 13, 2015

jquense commented Aug 17, 2015

jquense commented Aug 20, 2015

mtscout6 commented Aug 20, 2015

Choose a reason for hiding this comment

taion commented Aug 20, 2015

jquense commented Aug 21, 2015

mtscout6 commented Aug 21, 2015

mtscout6 commented Aug 21, 2015

mtscout6 commented Aug 21, 2015

mtscout6 commented Aug 21, 2015

taion commented Aug 21, 2015

AlexKVal commented Aug 21, 2015

AndersDJohnson commented Oct 11, 2015