Refactor `Modal.dispose` and add test #27455

iamandrewluca · 2018-10-16T23:59:08Z

Having variables initialised from start _isTransitioning is better.
Would be better to add an ~~no-undef eslint rule~~ to check for undefined variables use.
Reordered variables by priority at show and hide function entrance.

fix Modal.dispose method
add test for Modal.dispose
add _isTransitioning default value

no-undef - this rule is enabled by default, but does not what we want
no-use-before-define - kind of does what we want, but does not work for class members

Resolves #27456

Johann-S · 2018-10-17T06:27:02Z

It seems your change decrease our coverage 🤔 can you investigate why ?

Your changes looks good to me 👍

iamandrewluca · 2018-10-17T07:49:11Z

@Johann-S I don't have so much experience with coveralls, what does this means?
I think a test for dispose should be added.

ps: good talk https://www.youtube.com/watch?v=O2tsvUJT09U

Johann-S · 2018-10-17T08:19:04Z

Yep we have a lot of unit test we should add 😄
Coveralls show you where you have increased/decreased the coverage.

We don't want a 100% of coverage but at least 90% would be great 👍

iamandrewluca · 2018-10-18T22:31:23Z

@Johann-S what does this selector match?

bootstrap/js/src/modal.js

Line 200 in 5d834e0

$(window, document, this._element, this._backdrop).off(EVENT_KEY)

I want to test that line, but don't understand on what element event is turned off.
Is that a nested selector, or parallel selector?
Couldn't find any docs.

Johann-S · 2018-10-19T07:52:23Z

@iamandrewluca this line remove all the event listeners we attached during the use of our modal

iamandrewluca · 2018-10-19T08:06:26Z

@Johann-S yes, I understand this.
But it removes listeners on each one? window document element and backdrop? Or that will search for backdrop inside element inside document inside window? jQuery does not have such docs for multiple arguments constructor. It only has that one with context, but it works in reverse order of arguments.

jQuery(element1, element2)

This will try to find element1 inside element2.

Johann-S · 2018-10-19T08:16:30Z

Yep for me it should remove the event listeners on each elements and that's what we should check

Johann-S · 2018-10-20T16:57:02Z

js/tests/unit/modal.js

+
+    var $modal = $('<div id="modal-test"/>')
+      .bootstrapModal('show')
+      .bootstrapModal('hide')


I don't understand the need to use show/hide methods here 🤔

var $modal = $('<div id="modal-test"/>').bootstrapModal() assert.ok(typeof $modal.data('bs.modal') !== 'undefined', 'modal created') $modal.bootstrapModal('dispose') assert.ok(typeof $modal.data('bs.modal') === 'undefined', 'modal data object was disposed')

If you want to go further you can use a SinonJS spy to be sure dispose is called

From start bs.modal is undefined, then is set on show, then is removed again on hide

Disposing modal in show state is ambiguous. All listeners are removed, and you're stuck with an open modal on page.

The modal instance is created here: https://github.com/twbs/bootstrap/blob/94a0144b2cfa5d50b2976295da9d07465acb22ad/js/src/modal.js#L509
And me remove the data on dispose here: https://github.com/twbs/bootstrap/blob/94a0144b2cfa5d50b2976295da9d07465acb22ad/js/src/modal.js#L199

No need to show or hide

You mean I have just to call $('#myModal').modal() ?
This 2 are the same

$('#myModal').modal() // will call `show` method $('#myModal').modal('show') // will call `show` method

On tests page I get an error if I don't hide modal before dispose, when trying to click on something

QUnit.test('should dispose modal', function (assert) { assert.expect(1) var done = assert.async() var $modal = $('<div id="modal-test"/>') .bootstrapModal() .bootstrapModal('dispose') assert.ok(typeof $modal.data('bs.modal') === 'undefined', 'modal data object was disposed') done() })

Also if I don't hide, body will have the padding added when there is overflow.

Tested that :

QUnit.test('should dispose modal', function (assert) { assert.expect(2) var $el = $('<div id="modal-test"/>') $el.bootstrapModal() assert.ok($el.data('bs.modal') !== undefined) $el.bootstrapModal('dispose') assert.ok($el.data('bs.modal') === undefined) })

and it works fine on our unit tests

Tried to click any input on the tests page? you will get an error, and also body padding is added.
Tests are passing, but tests page looks weird.

That's not the purpose here, we test if our dispose methods do the job that's all, so we just have to check if the data is removed and if the listeners too.

I did the first check (check if the data is removed).
We did that on other tests for our Modal and it's fine.

If you want we can discuss about that on the Bootstrap Slack 😉

Having variables initialised from start `_isTransitioning` is better. Would be better to add an eslint rule to check for undeclared variables use. Reordered enter checks for `show` and `hide` by priority.

iamandrewluca · 2018-10-30T13:55:14Z

@Johann-S Done.
Some notes:

this is not valid jQuery

$(window, document, this._element, this._backdrop).off(EVENT_KEY)

this._backdrop does not have any events attached to it. this._dialog has.
After dispose only Event.CLICK_DATA_API should remain on page

ps: my next PR also will be on modal improvement 😏 (about fixed, sticky elements)

js/src/modal.js

Johann-S · 2018-10-30T14:01:09Z

js/tests/unit/modal.js

+    ].join('')).appendTo('#qunit-fixture')
+
+    $modal.on('shown.bs.modal', function () {
+      var data = $(this).data()


var modal = $(this).data('bs.modal')

Johann-S · 2018-10-30T14:07:14Z

js/src/modal.js

@@ -197,7 +198,21 @@ class Modal {
  dispose() {
    $.removeData(this._element, DATA_KEY)

-    $(window, document, this._element, this._backdrop).off(EVENT_KEY)
+    /**


Can you remove this comment please ? We don't need to keep the history in comment, we have git which handle that for us 😉

Johann-S · 2018-10-30T14:15:26Z

js/tests/unit/modal.js

+
+      assert.ok(typeof data['bs.modal'] === 'undefined', 'modal data object was disposed')
+
+      const modalEvents = [window, document, element, dialog]


Instead of doing that, you should use SinonJS spy to be sure we call .off on those elements

iamandrewluca changed the title ~~Add _isTransitioning default value~~ Add _isTransitioning default value for Modal Oct 17, 2018

Johann-S added js v4 labels Oct 17, 2018

iamandrewluca changed the title ~~Add _isTransitioning default value for Modal~~ refactor(Modal): add _isTransitioning default value for Modal Oct 18, 2018

Johann-S reviewed Oct 20, 2018

View reviewed changes

twbs deleted a comment from iamandrewluca Oct 22, 2018

iamandrewluca changed the title ~~refactor(Modal): add _isTransitioning default value for Modal~~ WIP: Add test for dispose method Oct 29, 2018

iamandrewluca changed the title ~~WIP: Add test for dispose method~~ WIP: Add test for Modal.dispose method Oct 29, 2018

iamandrewluca changed the title ~~WIP: Add test for Modal.dispose method~~ Add test for Modal.dispose method Oct 30, 2018

refactor(Modal): add _isTransitioning default value

a00aca2

Having variables initialised from start `_isTransitioning` is better. Would be better to add an eslint rule to check for undeclared variables use. Reordered enter checks for `show` and `hide` by priority.

iamandrewluca changed the title ~~Add test for Modal.dispose method~~ Refactor Modal.dispose and add test Oct 30, 2018

Johann-S requested changes Oct 30, 2018

View reviewed changes

test(Modal): check if modal is disposed

9a41a95

Johann-S approved these changes Oct 30, 2018

View reviewed changes

Johann-S merged commit bd28519 into twbs:v4-dev Oct 30, 2018

mdo mentioned this pull request Oct 30, 2018

v4.2.0 shiplist #26952

Closed

iamandrewluca deleted the patch-1 branch October 30, 2018 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Modal.dispose` and add test #27455

Refactor `Modal.dispose` and add test #27455

iamandrewluca commented Oct 16, 2018 •

edited

Loading

Johann-S commented Oct 17, 2018

iamandrewluca commented Oct 17, 2018 •

edited

Loading

Johann-S commented Oct 17, 2018

iamandrewluca commented Oct 18, 2018 •

edited

Loading

Johann-S commented Oct 19, 2018

iamandrewluca commented Oct 19, 2018

Johann-S commented Oct 19, 2018

Johann-S Oct 20, 2018

iamandrewluca Oct 21, 2018

iamandrewluca Oct 21, 2018

iamandrewluca Oct 21, 2018

Johann-S Oct 21, 2018

iamandrewluca Oct 24, 2018

Johann-S Oct 24, 2018

iamandrewluca Oct 24, 2018 •

edited

Loading

Johann-S Oct 24, 2018

Johann-S Oct 24, 2018

iamandrewluca commented Oct 30, 2018 •

edited

Loading

Johann-S Oct 30, 2018

Johann-S Oct 30, 2018

Johann-S Oct 30, 2018


		assert.ok(typeof data['bs.modal'] === 'undefined', 'modal data object was disposed')

		const modalEvents = [window, document, element, dialog]

Refactor Modal.dispose and add test #27455

Refactor Modal.dispose and add test #27455

Conversation

iamandrewluca commented Oct 16, 2018 • edited Loading

Johann-S commented Oct 17, 2018

iamandrewluca commented Oct 17, 2018 • edited Loading

Johann-S commented Oct 17, 2018

iamandrewluca commented Oct 18, 2018 • edited Loading

Johann-S commented Oct 19, 2018

iamandrewluca commented Oct 19, 2018

Johann-S commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamandrewluca Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamandrewluca commented Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `Modal.dispose` and add test #27455

Refactor `Modal.dispose` and add test #27455

iamandrewluca commented Oct 16, 2018 •

edited

Loading

iamandrewluca commented Oct 17, 2018 •

edited

Loading

iamandrewluca commented Oct 18, 2018 •

edited

Loading

iamandrewluca Oct 24, 2018 •

edited

Loading

iamandrewluca commented Oct 30, 2018 •

edited

Loading