-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
feat: make hammerjs optional #2280
Conversation
constructor() { | ||
super(); | ||
|
||
if (!this._hammer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you import isDevMode
from @angular/core
and then only log this warning if isDevMode()
is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -4,16 +4,25 @@ import {HammerGestureConfig} from '@angular/platform-browser'; | |||
/* Adjusts configuration of our gesture library, Hammer. */ | |||
@Injectable() | |||
export class MdGestureConfig extends HammerGestureConfig { | |||
private _hammer = typeof window !== 'undefined' ? (window as any).Hammer : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than making it any
, could you add a GestureManager
interface that just copies the members of HammerManager
that we're using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with any
here and a bit below, because it throws a TS compile error if HammerJS is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right- instead of dropping all the way down to any
, though, we can introduce out own interface and say (window as any).Hammer as GestureManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll look into the API.
5dffefb
to
adba735
Compare
Makes HammerJS and logs a warning if it's missing, instead of crashing the app.
adba735
to
0c7b8fb
Compare
Updated @jelbourn. I've replaced the HammerJS interfaces with our own stripped-down alternatives. |
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Makes HammerJS and logs a warning if it's missing, instead of crashing the app.