-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
add originalEvent to _clearMenus function #26343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this error: https://travis-ci.org/twbs/bootstrap/jobs/368542629#L1156
and you must add a unit test to cover this case
@Johann-S error was fixed |
Please add a unit test |
js/src/dropdown.js
Outdated
@@ -341,6 +341,10 @@ const Dropdown = (($) => { | |||
relatedTarget: toggles[i] | |||
} | |||
|
|||
if (event) { | |||
relatedTarget.original = event.originalEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relatedTarget.originalEvent
would be better
Why did you limit this event to click ? |
@Johann-S I wanted to avoid a collision with the name |
js/src/dropdown.js
Outdated
@@ -341,6 +341,10 @@ const Dropdown = (($) => { | |||
relatedTarget: toggles[i] | |||
} | |||
|
|||
if (event && event.type === 'click') { | |||
relatedTarget.mouseEvent = event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer: clickEvent
js/tests/unit/dropdown.js
Outdated
@@ -498,6 +498,39 @@ $(function () { | |||
$dropdown.trigger('click') | |||
}) | |||
|
|||
QUnit.test('should fire hide and hidden event with a mouseEvent', function (assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I you want to add this property only for click event, you should add a test which verify this property isn't added on other type of events
js/tests/unit/dropdown.js
Outdated
assert.notOk(e.clickEvent) | ||
}) | ||
.on('hidden.bs.dropdown', function (e) { | ||
assert.onotO(e.clickEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.onotO(e.clickEvent)
I don't think onotO
exist on assert
😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@mdo can I add new features in our next release ? (4.1.1) Or should I wait for 4.2 ?
@Johann-S: does this need to be documented in docs? |
yep that can be usefull, good catch @XhmikosR 👍 |
Closing for lack of replies. |
Hi @XhmikosR , why did you close this pull request? Is there any action that I have to do? Or somebody else? |
fixes #26336