Fix modal dismissal #702
Fix modal dismissal #702
Conversation
Codecov Report
@@ Coverage Diff @@
## master #702 +/- ##
=========================================
- Coverage 95.86% 95.67% -0.2%
=========================================
Files 129 129
Lines 6336 6352 +16
Branches 410 411 +1
=========================================
+ Hits 6074 6077 +3
- Misses 262 275 +13
Continue to review full report at Codecov.
|
|
I added @seanlip for the overall review and @davyrisso-at-google for the E2E test review. I used browser.sleep() because I could not get browser.wait() to work properly. It works as is, but I suspect using browser.wait() might be more robust? Love to get suggestions on how to make the tests better. |
I recommend using await browser.wait() with ExpectedConditions.presenceOf()/visibilityOf()/invisibilityOf(). E.g in pageObjects openModal() would become: this.openModal = async function() { and dismissModal(): this.dismissModal = async function() { Note that in the spec it is necessary to use await when calling both these methods, i.e: I am not sure if the current tests in specs are all necessary (e.g testing the presence/absence of the ng-hide class should be taken care of with ExpectedCondition.visibilityOf/invisibilityOf, maybe we should rather test the presence and content of the text in the modal? Let me know if this works! |
There was a problem hiding this comment.
Thanks @rabidbit, took a first pass.
FWIW, from a UI/UX perspective, I think I personally prefer the current behaviour (no animation on-close), mainly because it's faster. But that's not meant as an override -- it's just a data point for consideration. Feel free to overrule.
| /** | ||
| * Event handler to hide modal after transition animation ends. | ||
| */ | ||
| $scope.hideModalAfterAnimation = function(event) { |
There was a problem hiding this comment.
I'm not sure this needs to be defined here; it duplicates code in MonospaceDisplayModalService.
| }; | ||
|
|
||
| /** | ||
| * Close modal by calling MonospaceDisplayModalService.closeModal. |
There was a problem hiding this comment.
The function doesn't seem to be doing what this docstring says.
| /** | ||
| * Close modal by calling MonospaceDisplayModalService.closeModal. | ||
| */ | ||
| $scope.closeModal = function() { |
There was a problem hiding this comment.
The code here duplicates code in MonospaceDisplayModalDirective. In general try to strictly avoid duplicating code since that makes things harder to maintain and introduces the possibility of skew.
| * Close the modal. | ||
| * Event handler to hide modal after transition animation ends. | ||
| */ | ||
| $scope.hideModalAfterAnimation = function(event) { |
There was a problem hiding this comment.
Consider using evt instead of event throughout (which I think we do already).
(event is a global variable in some versions of IE, so typically I avoid using it.)
| $timeout(function() { | ||
| MonospaceDisplayModalService.hideModal(); | ||
| }, 0); | ||
| event.target.removeEventListener("transitionend", |
There was a problem hiding this comment.
nit: break after '(' if contents of (...) span more than one line
Here and elsewhere, use single quotes
|
OK. I'm happy to defer on UX. |
The dismiss button on the modal does not dismiss the modal in a smooth manner. Hitting the "Get Feedback" button also dismisses the modal abruptly (it just disappears).