Fix default timeouts in OC.Notification #16573
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tje jQuery object created through "$('#testArea .toastify')" will be
always defined even if no elements were found, so the check does not
really work; instead, it should be checked the number of elements found.
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
"showTemporary()" when a timeout was given was being tested along with the "show()" tests; now there are two separate tests when a timeout is given, one for "showTemporary()" and one for "show()". Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When no timeout was given "show()" used the default timeout of "OCP.Toast", which is 7 seconds instead of indefinitely as stated in the documentation of "show()". "showHtml()" should also indefinitely show the notification if no timeout is given, but due to the strict comparison the notification was indefinitely shown only when a timeout of 0 was explicitly given. Now both methods show the notification indefinitely (or until it is explicitly hidden) when no timeout is given. The unit tests did not catch this error because "showHtml()" had no tests (as before the move to Toastify it was called from "show()" and thus implicitly tested), and because "show()" verified that "hide()" was not called after some time; "hide()" is no longer called from "show()" since "OCP.Toast" is used internally, so the test always passed even if the notification was indeed hidden. Now the test is based on whether the element is found or not, and explicit tests were added too for "showHtml()". Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
juliusknorr
approved these changes
Jul 26, 2019
rullzer
approved these changes
Jul 28, 2019
Member
rullzer
left a comment
rullzer
left a comment
There was a problem hiding this comment.
More tests than code.awesome :)
delete-merged-branch
bot
deleted the
fix-default-timeouts-in-oc-notification
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a regression introduced in #15124
Before
OC.Notificationwas changed to use Toastifyshow()internally calledshowHtml(), which set the timeout to 0 if none was given. After the move to Toastifyshow()calledOCP.Toastdirectly without passing any timeout, so the default timeout ofOCP.Toast, which is 7 seconds, was used instead. Also, after the move to ToastifyshowHtml()passed the infinite timeout toOCP.Toast, but only if it was explicitly set to 0 due to using a strict comparison, so again when no timeout was given the notification was also hidden after 7 seconds. Now bothshow()andshowHtml()show the notification indefinitely (or untilhide()is explicitly called) if no timeout is given.Besides the fix itself I have also fixed and extended a bit the unit tests.