Shortcuts now check closeOnSelection to decise shouldDismissPopover

[Yonben]

Fixes #3338

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Correctly prevent DateRangePicker shortcuts from dismissing popovers

Reviewers should focus on:

Components on the way to Shortcuts are already getting the closeOnSelection props. I added it to Shortcuts in order for it to know if it should pass shouldDismissPopover to the MenuItem (which used to manually get the className).
Also first real PR, so I feel code style etc.. needs some improvement and would love constructive inputs.

Screenshot

N/A

Thanks!

[palantirtech]

Thanks for your interest in palantir/blueprint, @Yonben! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[adidahiya]

the behavior change in this PR is desirable, it seems to fix #3338:

however, after looking at this more closely, I think there is a deeper problem of how we treat the "dismiss" behavior for controlled vs. uncontrolled popovers. there seems to be a "bug" in our treatment of controlled popovers where the popover sometimes dismisses itself due to the Classes.POPOVER_DISMISS class even when the popover isOpen state is controlled by the consumer. I don't think we want this; it can lead to confusing app state. therefore, I propose that we change handlePopoverClick in popover.tsx to only set open state if the component is uncontrolled (this may be a small breaking change, but only if someone was abusing this bug in the API...)

[Yonben]

@adidahiya I think that we should first fix the undesirable behaviour and then reiterate and discuss/change the API to fit the problem better. In the meantime, the component behaves incorrectly and that should be priority imo.

[adidahiya]

In the meantime, the component behaves incorrectly and that should be priority imo.

yes, but I don't want to do that if it means adding a new prop to the API. increasing the API surface area to fix the bug is not a good development tradeoff for this project.