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

fix(core): remove hammerjs from library #5980

Merged
merged 5 commits into from
Jul 15, 2021
Merged

Conversation

N1XUS
Copy link
Contributor

@N1XUS N1XUS commented Jul 14, 2021

Please provide a link to the associated issue.

Closes #4132

Please provide a brief summary of this pull request.

Removed hammerjs library from fundamental-ngx/core and refactored carousel component

Please check whether the PR fulfills the following requirements

Documentation checklist:

@netlify
Copy link

netlify bot commented Jul 14, 2021

✔️ Deploy Preview for fundamental-ngx ready!

🔨 Explore the source changes: fbda745

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-ngx/deploys/60f0461f10776d0007626f91

😎 Browse the preview: https://deploy-preview-5980--fundamental-ngx.netlify.app

Comment on lines +7 to +19
.fd-carousel__content {
> div {
touch-action: pan-x;
user-select: none;
-webkit-user-drag: none;
-webkit-tap-highlight-color: rgba(0, 0, 0, 0);

&.fd-carousel__content--horizontal {
touch-action: pan-y;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to fundamental-styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created separate bug in fundamental-styles to reflect the carousel component's layout from fundamental-ngx: SAP/fundamental-styles#2574

Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

minor stuff, the code looks great
Also Travis is failing

/** @hidden */
private _dragStarted = false;

/** An RxJS Subject that will kill the data stream upon component’s destruction (for unsubscribing) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be hidden

this._hammer.on('panstart', () => this._handlePanStart());
this._hammer.on('panend', (event) => this._handlePanEnd(event.deltaX));
}
private _subscribeToEvents(
Copy link
Contributor

Choose a reason for hiding this comment

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

/** @hidden */

@N1XUS N1XUS requested a review from InnaAtanasova July 14, 2021 18:25
@N1XUS N1XUS self-assigned this Jul 14, 2021
@N1XUS N1XUS added bug Something isn't working denoland-porto Denoland sprint 66 Porto - bugfixing enhancement New feature or request labels Jul 14, 2021
@N1XUS N1XUS added this to the Sprint 66 - Porto milestone Jul 14, 2021
@N1XUS N1XUS requested a review from kavya-b July 15, 2021 18:24
@droshev droshev merged commit 690eb48 into main Jul 15, 2021
@droshev droshev deleted the fix/4132-remove-hammerjs branch July 15, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working denoland-porto Denoland sprint 66 Porto - bugfixing enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: dependency on CommonJS modules
5 participants