Skip to content

refactor: clean up remaining hammer.js usages #17268

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 3, 2019
Merged

refactor: clean up remaining hammer.js usages #17268

merged 1 commit into from
Oct 3, 2019

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 2, 2019

Now that none of the components use Hammer.js anymore, we can clean up the remaining usages from the project.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 2, 2019
@crisbeto
refactor: clean up remaining hammer.js usages
0d7a183 
Now that none of the components use Hammer.js anymore, we can clean up the remaining usages from the project.
crisbeto
crisbeto commented Oct 2, 2019
View reviewed changes
src/material/core/gestures/gesture-config.ts
* Adjusts configuration of our gesture library, Hammer.
* @deprecated No longer being used. To be removed.
* @breaking-change 10.0.0
*/
@Injectable()
export class GestureConfig extends HammerGestureConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we can also get rid of all of this code and the one in gesture-annotations.ts, but it got exported through material/core so it's technically public API. If we don't consider it public we can remove it now, otherwise we need to wait for v10.

Copy link
Member

Choose a reason for hiding this comment

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

I think a few people actually provide the GestureConfig manually. e.g. to work around a bug: #4595 (comment), or if they use it to access the slide or longpress events.

I think the impact is very low if we remove it, but I could also see waiting for it until we have worked out the migration schematic.

Copy link
Member

Choose a reason for hiding this comment

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

There are ~18 references to GestureConfig in Google. I think we need need some more intelligent schematic work here. Let's discuss at the team meeting tomorrow.

@crisbeto crisbeto marked this pull request as ready for review October 2, 2019 10:25
@crisbeto crisbeto requested review from devversion, jelbourn, mmalerba and a team as code owners October 2, 2019 10:25
@crisbeto crisbeto added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: major This PR is targeted for the next major release labels Oct 2, 2019
devversion
devversion approved these changes Oct 2, 2019
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Nice 🎉

tools/public_api_guard/material/core.d.ts
@@ -197,8 +197,7 @@ export declare const MAT_OPTION_PARENT_COMPONENT: InjectionToken<MatOptionParent
export declare const MAT_RIPPLE_GLOBAL_OPTIONS: InjectionToken<RippleGlobalOptions>;

export declare class MatCommonModule {
constructor(sanityChecks: any, _hammerLoader?: HammerLoader | undefined);
_checkHammerIsAvailable(): void;
constructor(sanityChecks: any);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it could be a breaking change if someone manually passes a HammerLoader. I know that this is a rare edge case, but better to talk about it than accidentally missing it.

I'm happy leaving it like that, but could also see having a fake parameter like you did in some other PR (with injecting ElementRef)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point. My thinking when removing it was that it was optional already.

Copy link
Member

Choose a reason for hiding this comment

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

Of all the things people could manually instantiate or extend, MatCommonModule would be the strangest. I'm okay just removing this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just wanted to raise awareness. As mentioned above, I'm fine with that either.

src/material/core/gestures/gesture-config.ts
* Adjusts configuration of our gesture library, Hammer.
* @deprecated No longer being used. To be removed.
* @breaking-change 10.0.0
*/
@Injectable()
export class GestureConfig extends HammerGestureConfig {
Copy link
Member

Choose a reason for hiding this comment

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

I think a few people actually provide the GestureConfig manually. e.g. to work around a bug: #4595 (comment), or if they use it to access the slide or longpress events.

I think the impact is very low if we remove it, but I could also see waiting for it until we have worked out the migration schematic.

@mmalerba mmalerba added this to the 9.0.0 milestone Oct 2, 2019
jelbourn
jelbourn approved these changes Oct 2, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, I think this can go in now and we can follow up with our next steps after tomorrow's team meeting

tools/public_api_guard/material/core.d.ts
@@ -197,8 +197,7 @@ export declare const MAT_OPTION_PARENT_COMPONENT: InjectionToken<MatOptionParent
export declare const MAT_RIPPLE_GLOBAL_OPTIONS: InjectionToken<RippleGlobalOptions>;

export declare class MatCommonModule {
constructor(sanityChecks: any, _hammerLoader?: HammerLoader | undefined);
_checkHammerIsAvailable(): void;
constructor(sanityChecks: any);
Copy link
Member

Choose a reason for hiding this comment

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

Of all the things people could manually instantiate or extend, MatCommonModule would be the strangest. I'm okay just removing this.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 2, 2019
@mmalerba mmalerba merged commit c4f2174 into angular:master Oct 3, 2019
@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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion approved these changes

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@jelbourn jelbourn

@devversion devversion

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: major This PR is targeted for the next major release
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@crisbeto @jelbourn @devversion @googlebot @mmalerba