fix(dropdown): use mousedown to close dropdown #1815
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1815 +/- ##
==========================================
+ Coverage 64.77% 65.23% +0.45%
==========================================
Files 156 156
Lines 2950 2957 +7
Branches 810 812 +2
==========================================
+ Hits 1911 1929 +18
+ Misses 752 745 -7
+ Partials 287 283 -4
Continue to review full report at Codecov.
|
…ause doing it in dropdown assume clickout mixin can never be used by other class directly
|
@rinick. Note multiple components use the Have you ensured that this change does not impact other components? |
|
There are also a few conflicts with the test fixture HTML (and indirectly with the fixture test script). Could you also adjust your test HTML and JavaScript? |
|
And would you be able to merge the latest |
tmorehouse
added
Status: Changes Needed
| if (!this._clickOutElement && typeof document !== 'undefined') { | ||
| this._clickOutElement = document.documentElement | ||
| this._clickOutElement.addEventListener('mousedown', this._clickOutListener) | ||
| } |
There was a problem hiding this comment.
When adding an event listener, if the event handler is already attached, reattaching it will not actually rea attach it (as long as the function reference is the same as the one that was called).
So storing the document element on the component is not necessary.
A simple check for the existence of typeof document !== 'undefined' is needed, then either add or remove the listener when needed.
There was a problem hiding this comment.
You could also use the eventOn and eventOff functions from utils/dom.js to add and remove the events (this would help with code minification)
just add import { eventOn, eventOff } from '../utils/dom at the top, and then:
eventOn(document.documentElement, 'mousedown', this._this._clickOutListener)
eventOff(document.documentElement, 'mousedown', this._this._clickOutListener)| } | ||
| }, | ||
| methods: { | ||
| listenClickOut () { | ||
| if (!this._clickOutElement && typeof document !== 'undefined') { | ||
| this._clickOutElement = document.documentElement |
There was a problem hiding this comment.
Why store a reference of the document element on the component instance?
There was a problem hiding this comment.
We could store a reference to the document element in a constant at the top of the mixin
const docEl = document ? document.documentElement : nulland then use the following:
beforeDestroy() {
if (docEl) {
eventOff(docEl, 'mousedown', this._clickoutListener)
}
if (this.clickOutListener) {
this.clickOutListener()
}
},
methods: {
listenClickOut () {
if (docEl) {
eventOn(docEl, 'mousedown', this._clickoutListener)
}
}
}
|
Closing as PR #2159 fixes the issues wrt clickout listener. |
dropdown wont close when event gets stopPropagation()
since click event stopPropagation() is used in several componenets in bootstrap-vue, it's better to switch to mousedown event to close popup.
resolves #1814
besides that, clickout listener is now only added on demand, resolves #1817