Dropdown fix

[emplums]

This PR removes an out of place preventDefault that was stopping clicks on dropdown items from registering. I am honestly not 100% sure why I felt the need to add this in the first place but I tested out several different scenarios with different dropdowns on the page being open/etc and this should be safe to remove 👍

Closes: #491 , thanks for the heads up @nicholaai!

[colebemis]

This change looks good to me. I can't think of any unintended consequences to this change off of the top of my head.

I have a more general question about the Dropdown API though. What's our recommended way of handling "selection" of a dropdown item? Dropdown.Item currently renders an li, so it wouldn't be accessible to add an onClick handler. I suppose we could the as prop to render a button instead of an li but the browser button styles break the dropdown item styles. Should we add an onSelect prop to Dropdown.Item like Reach UI? https://ui.reach.tech/menu-button

[emplums]

@colebemis good catch! I was wondering that too when I was playing around with this today, but thought it was taken care of because that's how the dropdowns in blueprints function, BUT turns out we're creating a totally custom dropdown instead of using this one 😂 I need to spend some time thinking about the right API here

[shawnbot]

It looks like preventDefault() was part of the introductory PR of this component in #48, and I think the problem I was trying to solve was that without it, the click triggers the browser-native toggling of the <details> element, so the DOM element's open state conflicts with open state we're tracking in JS. This isn't a problem unless you pass the open boolean prop:

I wonder if the right thing to do here is to actually use a ref and have the open state reflected by the value of the DOM element's open property? 🤔

[shawnbot]

@colebemis and I talked about this and I agree that we should probably adopt the uncontrolled components convention by having a defaultOpen prop rather than open in our Details component, then respond to toggle events to update the state accordingly. This has the benefit of playing nicely with other JS behaviors (and even browser extensions) that might open or close details outside of our control.

[emplums]

@colebemis refreshed a bit on why it's built with a list there - we're expecting users to add links or buttons as children of the <li> elements. The lists help with keyboard navigation and is the standard pattern for a11y dropdowns!

[emplums]

I think this is ready for a final review now!

[emplums]

Ok, so after some rabbit hole adventures, this will need to introduce a breaking change. In order to fix the original issue of dropdown menu item clicks being swallowed by the prevent default in the open/close functionality, I needed to introduce a onToggle click handler to the wrapper element. Doing this breaks the render prop method because we then would have two toggle tracking functions listening at the same time.

I'm okay with this change because it seems a little silly to have 3 ways of using this component, and if the user really needs some special functionality to happen onClick or onToggle for the details element, they could add that behavior in separately.