Remove popover dismiss class from DateRangePicker shortcuts #3339
Conversation
|
Thanks for your interest in palantir/blueprint, @mjbcopland! 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. |
There was a problem hiding this comment.
@mjbcopland thanks for the PR! please change this so the dismiss class is only applied if closeOnSelection={true}
| @@ -46,7 +46,7 @@ export class Shortcuts extends React.PureComponent<IShortcutsProps> { | |||
|
|
|||
| const shortcutElements = shortcuts.map((s, i) => ( | |||
| <MenuItem | |||
| className={Classes.POPOVER_DISMISS_OVERRIDE} | |||
| shouldDismissPopover={false} | |||
There was a problem hiding this comment.
mm it's not entirely this simple. can we actually make this value correspond to the closeOnSelection prop?
so if you enable that prop then clicking shortcuts will close, otherwise they won't.
| @@ -1169,6 +1169,11 @@ describe("<DateRangePicker>", () => { | |||
| assert.equal(onChangeSpy.firstCall.args[0][0] as Date, startTime); | |||
| }); | |||
|
|
|||
| it("shortcuts do not dismiss popovers", () => { | |||
There was a problem hiding this comment.
let's test that this feature is tied to closeOnSelection
|
@mjbcopland also please sign the CLA 😄 i cannot merge until you accept it. |
|
@mjbcopland are you able to sign the CLA? |
|
What could we do to get that fix in if the original writer doesn't sign? Could I fork and do it again? |
|
@Yonben yes you can open a new PR |
Fixes #3338
Checklist
Changes proposed in this pull request:
Correctly prevent DateRangePicker shortcuts from dismissing popovers
Reviewers should focus on:
Shortcuts are implemented using MenuItems, which receive the POPOVER_DISMISS class by default. In order to prevent this behaviour, DateRangePicker adds the POPOVER_DISMISS_OVERRIDE class. However, when an element has both classes, the former takes priority and the override has no effect. This change uses the shouldDismissPopover prop of MenuItem to achieve the desired behaviour.
Screenshot
N/A