Skip to content

Conversation

@crisbeto
Copy link
Member

Makes HammerJS and logs a warning if it's missing, instead of crashing the app.

@crisbeto crisbeto requested a review from jelbourn December 19, 2016 16:15
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 19, 2016
constructor() {
super();

if (!this._hammer) {
Copy link
Member

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?

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.

/* Adjusts configuration of our gesture library, Hammer. */
@Injectable()
export class MdGestureConfig extends HammerGestureConfig {
private _hammer = typeof window !== 'undefined' ? (window as any).Hammer : null;
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Makes HammerJS and logs a warning if it's missing, instead of crashing the app.
@crisbeto
Copy link
Member Author

Updated @jelbourn. I've replaced the HammerJS interfaces with our own stripped-down alternatives.

@jelbourn
Copy link
Member

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Dec 20, 2016
@jelbourn jelbourn merged commit 28691ca into angular:master Dec 20, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants