Skip to content
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

Update TemplateStamp event listen param types from Node to EventTarget. #5320

Merged
merged 4 commits into from
Aug 11, 2018

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Aug 10, 2018

Allows passing e.g. window, which I assume is valid here?

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis is failing, as GestureEventListeners uses the same types and thus needs this change as well. I think this is a fine change nonetheless.

@aomarks
Copy link
Member Author

aomarks commented Aug 10, 2018

Travis is failing, as GestureEventListeners uses the same types and thus needs this change as well. I think this is a fine change nonetheless.

Fixed. Glad we're testing the types compile in CI now!

@aomarks
Copy link
Member Author

aomarks commented Aug 10, 2018

Fixed. Glad we're testing the types compile in CI now!

Had to fix gestures.js too. We set .style in that lib, which window doesn't have. I added an instanceof check. @azakus can you confirm this make sense?

* @param {string} value Touch action value
* @return {void}
*/
export function setTouchAction(node, value) {
if (HAS_NATIVE_TA) {
if (HAS_NATIVE_TA && node instanceof Node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a check for Element to ensure the style property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (HTMLElement actually)

Copy link
Member

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aomarks aomarks merged commit e8167f7 into master Aug 11, 2018
@aomarks aomarks deleted the event-target branch August 11, 2018 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants