Added autoClose behaviour for dropdowns - #5873
Conversation
kyletsang
left a comment
kyletsang
left a comment
There was a problem hiding this comment.
Overall, looks good. Thanks for taking this on. Left a few comments.
| focusFirstItemOnShow?: boolean | 'keyboard'; | ||
| onSelect?: SelectCallback; | ||
| navbar?: boolean; | ||
| autoClose: 'true' | 'outside' | 'inside' | 'false'; |
There was a problem hiding this comment.
Let's change the string true/false to a boolean
There was a problem hiding this comment.
Yes, I thought of that as well. Wasn't sure about the consistency but I will change that.
| ) { | ||
| source = 'rootClose'; | ||
|
|
||
| const noOuterCloseModes = |
There was a problem hiding this comment.
Might be worth leaving comments about click what elements triggers the closing of the dropdown so it's more clear
| }); | ||
|
|
||
| describe('autoClose behaviour', () => { | ||
| const createDocumentListenersMock = () => { |
There was a problem hiding this comment.
Do we need this? We have other methods in other tests to trigger click events.
There was a problem hiding this comment.
I didn't notice any method concerned with simulating clicks outside of the component. Correct me if I'm wrong. Should we keep the outside click methods?
There was a problem hiding this comment.
We did use simulant.fire(document.body, 'click'); inside an Offcanvas test. Can we try that and see if it works for your test?
| @@ -0,0 +1,15 @@ | |||
| <> | |||
| {[true, 'outside', 'inside', false].map((autoClose, index) => ( | |||
There was a problem hiding this comment.
Wonder if we should rearrange/reword the examples so it mirrors upstream Bootstrap examples:
https://getbootstrap.com/docs/5.0/components/dropdowns/#auto-close-behavior
I got thrown off with the autoclose behavior for "inside" and "outside" differing from Bootstrap's docs, but turns out it was an issue of ordering/wording.
|
Looks good to me - left 1 comment regarding docs demo. @jquense, would be good to get your eyes on this. |
jquense
left a comment
jquense
left a comment
There was a problem hiding this comment.
LGTM, happy with how this was relatively straightforward to implement outside of react-overlays this is
|
Thanks! |
3 participants
More about the autoClose behaviour here.
Additionally to the implementation, I've added
Preview: https://deploy-preview-5873--react-bootstrap.netlify.app/