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

Always use CustomEvent polyfill #668

Merged
merged 2 commits into from
Apr 9, 2020
Merged

Always use CustomEvent polyfill #668

merged 2 commits into from
Apr 9, 2020

Conversation

oshi97
Copy link
Contributor

@oshi97 oshi97 commented Apr 9, 2020

This commit changes DOM.trigger to always use our custom event
polyfill, instead of looking in the window first to see if
CustomEvent exists as a function.

Consulting was running into issues with ie11 saying that
it doesn't know how to use the Event constructor. This likely
could have also been fixed by changing return new Event to
return new CustomEvent, however it feels safer (to me)
to always rely on our own polyfill, instead of potentially
using somebody else's polyfill.

J=SPR-1979
TEST=manual
Went on ie11, checked that adding a CustomEvent function onto the
window manually through the dev tools causes the error consulting
was seeing. This was checked by typing into the searchbar,
since the searchcomponent uses DOM.trigger.
Then, removed code and checked that there were no errors,
and autocomplete was displaying values that I could click on
to run a search. Also checked on chrome.

This commit changes DOM.trigger to always use our custom event
polyfill, instead of looking in the window first to see if
CustomEvent exists as a function.

Consulting was running into issues with ie11 saying that
it doesn't know how to use the Event constructor. This likely
could have also been fixed by changing `return new Event` to
`return new CustomEvent`, however it felt more robust to me
to always rely on our own polyfill, instead of something
somebody using the sdk adds.

TEST=manual
Went on ie11, checked that adding a CustomEvent function onto the
window manually through the dev tools causes the error consulting
was seeing. This was checked by typing into the searchbar,
since the searchcomponent uses DOM.trigger.
Then, removed code and checked that there were no errors,
and autocomplete was displaying values that I could click on
to run a search. Also checked on chrome.
@oshi97 oshi97 requested a review from creotutar April 9, 2020 13:09
@oshi97 oshi97 mentioned this pull request Apr 9, 2020
Copy link
Contributor

@alexisgrow alexisgrow left a comment

Choose a reason for hiding this comment

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

LGTM!

@oshi97 oshi97 merged commit 9870fcc into v0.13.3 Apr 9, 2020
@oshi97 oshi97 deleted the remove-event-constructor branch April 9, 2020 18:58
alexisgrow added a commit that referenced this pull request Apr 21, 2020
tmeyer2115 pushed a commit that referenced this pull request Apr 24, 2020
* Always use CustomEvent polyfill

This commit changes DOM.trigger to always use our custom event
polyfill, instead of looking in the window first to see if
CustomEvent exists as a function.

Consulting was running into issues with ie11 saying that
it doesn't know how to use the Event constructor. This likely
could have also been fixed by changing `return new Event` to
`return new CustomEvent`, however it felt more robust to me
to always rely on our own polyfill, instead of something
somebody using the sdk adds.

TEST=manual
Went on ie11, checked that adding a CustomEvent function onto the
window manually through the dev tools causes the error consulting
was seeing. This was checked by typing into the searchbar,
since the searchcomponent uses DOM.trigger.
Then, removed code and checked that there were no errors,
and autocomplete was displaying values that I could click on
to run a search. Also checked on chrome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants