♿ 🐛 Show Carousel control button when mouse is detected #30765
Conversation
|
I couldn't find a related test. Open to test ideas. |
There was a problem hiding this comment.
Thanks for making this contribution, LGTM!
For testing, touch is not yet supported in e2e -- this thread also discusses this, though I'm not sure this counts as a unique use case since this and the previous conversation came up from testing carousel's hidden controls feature. I'm not sure if visual-diff tests support this, but that would also be a good route for confirming whether or not the buttons are there.
In the mean time, a unit test should be sufficient. I'd recommend looking at test/unit/test-input.js for firing the onMouseDetected states and checking the classes accordingly.
For manual testing, there is also the test procedures outlined in the original issue. :)
| @@ -193,6 +200,21 @@ class AmpCarousel extends AMP.BaseElement { | |||
| // Need to wait for slides to exist first. | |||
| this.carousel_.goToSlide(Number(this.element.getAttribute('slide') || '0')); | |||
| // Signal for runtime to check children for layout. | |||
|
|
|||
| if ( | |||
| isAmp4Email(this.element.ownerDocument) || | |||
There was a problem hiding this comment.
amp-carousel:0.2 is not allowed in email yet, so this check is not necessary.
There was a problem hiding this comment.
Ah! that explains, I was wonder why we didn't have the check here : )
| * Whether to show control buttons | ||
| * @private {boolean} | ||
| */ | ||
| this.alwaysShowControls_ = false; |
There was a problem hiding this comment.
suggestion: We can name this showControls instead of alwaysShowControls, since it can change
|
|
|
The unit test requires me to mock too much events and is not very helpful imho. If possible I agree visual diff test would be helpful, but it would still require us to fake the mousemove and also touch event. Tested manually |
…0765) * show carousel control buttons when mouse event detected * show carousel control buttons when mouse is detected * recommend changes
Closes #29560