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

Make <RootCloseWrapper /> hide onmousedown instead of onclick #95

Merged
merged 3 commits into from
Jul 8, 2016

Conversation

glenjamin
Copy link
Contributor

As discussed in #92

Unlike modal there's no background overlay, so click() is a bit late if the user is doing an interaction that starts onmousedown like dragging or clicking another button.

In order to run the examples from a fresh working tree I had to peg the component-playground version.

I've done some testing of this, I can't find any problems so far. The existing discussions about mousedown problems I've found have all been related to Modals and the background overlay.

@taion
Copy link
Member

taion commented Jun 8, 2016

Thanks for the PR. To get this merged, let's make this configurable instead.

By default, most actions of this style do trigger on click – see e.g. the dropdowns on the GitHub page, or default TWBS dropdowns. As such, it's not correct to make our root close wrapper only trigger on mouse down.

@glenjamin
Copy link
Contributor Author

Sounds reasonable, what should the default be?

Some more data points: Gmail is mousedown, Google plus is click, Facebook is click, slack is mousedown

At a guess I'd say websites are more likely to be click, applications more likely to be down - although I think using mousedown over click doesn't really provide any/many downsides, but using click over mousedown can.

Any component that uses <RootCloseWrapper /> would also need to be configurable then.

@glenjamin
Copy link
Contributor Author

There's a chance this might fix #64 as well

@glenjamin
Copy link
Contributor Author

The diff will be much more readable without whitespace. You can add ?w=1 on the querystring if you didn't already know.

@jquense
Copy link
Member

jquense commented Jun 8, 2016

You can add ?w=1 on the querystring if you didn't already know.

omg magic 🌟

@taion
Copy link
Member

taion commented Jul 8, 2016

BTW, where we got click in the first place was that this was what Bootstrap (v3) did (thought v4 does click as well).

@jquense pointed out that GitHub splits the difference – overlays close on click, but there's a backdrop-style element that prevents interaction with the rest of the page while the dropdown is shown.

So given all that I think the right default here is click.

@glenjamin
Copy link
Contributor Author

Reckon we're good to merge if I flip the default to click then?

We'd then need to add the prop to consuming components in React Bootstrap

@taion
Copy link
Member

taion commented Jul 8, 2016

I wonder if we can just hijack the existing rootClose prop to accept a string in addition to a boolean.

Unlike modal there's no overlay, so click() is a bit late
if the user is doing an interaction that starts onmousedown
like dragging or clicking another button
@glenjamin
Copy link
Contributor Author

This should be good to merge now, rebased off master and made click the default.

@taion
Copy link
Member

taion commented Jul 8, 2016

LGTM

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

Successfully merging this pull request may close these issues.

3 participants