-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
[V4] Throw error when a plugin is in transition #17823
Conversation
As I said because of this lines : https://github.com/twbs/bootstrap/blob/v4-dev/js/src/util.js#L48-L50 I can't make unit tests with QUnit. So, Should I try in differents html files in the visual folders ? |
Yes, please add tests for this in one new HTML visual test file. |
@@ -53,6 +53,21 @@ | |||
<script src="../vendor/jquery.min.js"></script> | |||
<script src="../../dist/util.js"></script> | |||
<script src="../../dist/carousel.js"></script> | |||
<script type="text/javascript"> |
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 remove the type="text/javascript"
for consistency with the rest of the project
Done ! If you any feedbacks let me know |
$('#btnOne').tooltip('hide').off('shown.bs.tooltip') | ||
}) | ||
} | ||
alert('No error thrown for : testTooltipTransitionError') |
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.
This will always alert, no matter whether an error was thrown.
You need to keep a boolean variable to track whether an error was thrown, and then put this alert()
inside an if
that uses that variable.
Also, this has merge conflicts. |
I made changes in Sorry to ask @cvrebert but how can I get the last updates of the branch |
It should just be a matter of pulling and rebasing like you've done in the past. |
I've just learned this command Now they are no conflicts, if you want I can squash my commits |
Sorry to PING you @cvrebert I know you have a lot of works to do but... Yes I'm a impatient man 😄 and I love contributing for Bootstrap |
This has some conflicts now, but if it gets rebased and is good to go, I can get it merged. |
I'll rebase my PR soon thanks you ❤️ |
Done @mdo 💪 |
So, uh, we have more conflicts. 😆 Can I get one more rebase? |
if (this._isTransitioning) { | ||
throw new Error('Tooltip is transitioning') | ||
} | ||
let complete = () => { |
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.
'complete' is never reassigned. Use 'const' instead prefer-const
Done @mdo 👍 please tel me it's the last time 😆 |
We did it! Thanks again @Johann-S! <3 |
I wonder if there is a way to force stop transitions before hiding? When using Bootstrap with Angular/Knockout, etc. and dynamically binding Tooltip/Modal/etc., then there needs to be a way to quickly tear-down and re-initialise these plugins. |
* js-transitioning-1: Add callout regarding async methods and add some details to some JS methods Add callout for JS asynchronous modules Fix mention of transition.js => util.js Mention that JS methods are asynchronous in the Javascript docs Make modal, like other component, ignore command when transitioning Do not re-add the tooltip to the document if it’s already there. Avoid the tooltip suddenly disappearing and reappearing when hovering on and off Revert twbs#17823 # Conflicts: # js/src/modal.js # js/src/tooltip.js
Hi,
Feature #17800
I'll try to add some unit tests but I'm not sure it will works very well because of transition support isn't enable when tests are run with Grunt