Skip to content

✨ Bento amp-image-slider#35783

Merged
dethstrobe merged 98 commits intoampproject:mainfrom
rtCamp:add/bento/amp-image-slider
Apr 12, 2022
Merged

✨ Bento amp-image-slider#35783
dethstrobe merged 98 commits intoampproject:mainfrom
rtCamp:add/bento/amp-image-slider

Conversation

@AnuragVasanwala
Copy link
Copy Markdown
Contributor

@AnuragVasanwala AnuragVasanwala commented Aug 24, 2021 •
edited
Loading

Goal of the PR is to create Bento amp-image-slider.

Checklist

  • Add amp-image-slider
  • Add types
  • Add type annotations
  • Add test case
  • Add npm: true and wrapper : "bento" to bundles.config.extension.json
  • Update Storybook - AMP and Preact
  • latestVersion precedes options
  • Lint, Prettify new amp-image-slider

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Aug 26, 2021

This pull request introduces 2 alerts when merging ae4a181 into 84ce446 - view on LGTM.com

new alerts:

  • 2 for Variable not declared before use

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Sep 1, 2021

This pull request introduces 2 alerts when merging 41dc19f into 9779166 - view on LGTM.com

new alerts:

  • 2 for Variable not declared before use

@caroqliu caroqliu self-requested a review September 7, 2021 14:32
caroqliu
caroqliu previously requested changes Sep 7, 2021
View reviewed changes
AnuragVasanwala and others added 13 commits September 8, 2021 18:21
@AnuragVasanwala
Merge branch 'ampproject:main' into add/bento/amp-image-slider
d292da9
@AnuragVasanwala @caroqliu
Added "npm": true for build-system compilation config
39de8ad 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala @caroqliu
🚮 Removed validator file until rules are added
5c012d9 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala @caroqliu
🚮 Removed unnecessary example code
43e83bb 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala @caroqliu
🚮 Removed copyright header from all files
bc296ff 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala
♻️ Cleanup
4283c2b
@AnuragVasanwala
Merge branch 'ampproject:main' into add/bento/amp-image-slider
eab6de8
@AnuragVasanwala @caroqliu
🚮 Renoved Services
9272d49 
Services should not be used in the Preact component.

Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala
Merge branch 'add/bento/amp-image-slider' of https://github.com/rtCam…
d2c1f15 
…p/amphtml into add/bento/amp-image-slider
@AnuragVasanwala
🧪 Experiment commit, 🚮 Remvoed test console.log
fa1b261
@AnuragVasanwala @caroqliu
🖍 Added and Formatted JSS & CSS
09b95e5 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@AnuragVasanwala
🐛 Minor fixes for JSS
c50a926
@AnuragVasanwala @caroqliu
♻️ shouldHintReappear renamed to more precise name repeatHint
867c03f 
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@caroqliu caroqliu self-requested a review September 15, 2021 19:56
@AnuragVasanwala
Merge branch 'ampproject:main' into add/bento/amp-image-slider
f51014e
@AnuragVasanwala
🧪 Experimental Test: DOM APIs to JSX
758ef23 
Preact component should not be creating any elements with DOM APIs. Preferring JSX instead. This is an experimental code for my storybook test.
@AnuragVasanwala
Merge branch 'add/bento/amp-image-slider' of https://github.com/rtCam…
45fb874 
…p/amphtml into add/bento/amp-image-slider
dethstrobe
dethstrobe reviewed Jan 27, 2022
View reviewed changes
dethstrobe
dethstrobe reviewed Jan 27, 2022
View reviewed changes
dethstrobe
dethstrobe reviewed Jan 27, 2022
View reviewed changes
dethstrobe
dethstrobe reviewed Jan 27, 2022
View reviewed changes
extensions/amp-image-slider/1.0/component.js Outdated
Comment on lines +377 to +380
unlisten(unlistenMouseMove?.current);
unlisten(unlistenMouseUp?.current);
unlistenMouseMove.current = listen(window, 'mousemove', onMouseMove);
unlistenMouseUp.current = listen(window, 'mouseup', onMouseUp);
Copy link
Copy Markdown
Contributor

@dethstrobe dethstrobe Jan 27, 2022

Choose a reason for hiding this comment

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

If you just instantly assigning the ref, why unlist it beforehand?

Copy link
Copy Markdown
Contributor

@deepaklalwani97 deepaklalwani97 Jan 31, 2022

Choose a reason for hiding this comment

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

We have ported this logic from the 0.1 version component.

amphtml/extensions/amp-image-slider/0.1/amp-image-slider.js

Lines 419 to 433 in 556e94a

// In case, clear up remnants
// This is to prevent right mouse button down when left still down
this.unlisten_(this.unlistenMouseMove_);
this.unlisten_(this.unlistenMouseUp_);
this.unlistenMouseMove_ = listen(
this.win,
'mousemove',
this.onMouseMove_.bind(this)
);
this.unlistenMouseUp_ = listen(
this.win,
'mouseup',
this.onMouseUp_.bind(this)
);

I have updated the above logic with this one.

if ( unlistenMouseMove.current === null ) {
  unlistenMouseMove.current = listen(window, 'mousemove', onMouseMove);
}
if ( unlistenMouseUp.current === null ) {
  unlistenMouseUp.current = listen(window, 'mouseup', onMouseUp);
}

Please guide me If I am missing anything

Copy link
Copy Markdown
Contributor Author

@AnuragVasanwala AnuragVasanwala Apr 6, 2022

Choose a reason for hiding this comment

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

@dethstrobe Please let us know if we are missing something.

deepaklalwani97 and others added 13 commits January 31, 2022 15:39
@deepaklalwani97
Fix unlisten event function logic
dec6486
@AnuragVasanwala
Merge branch 'ampproject:main' into add/bento/amp-image-slider
869e06d
@AnuragVasanwala
♻️ Minor fix, lint and prettify
9f2d9db
@AnuragVasanwala
Merge branch 'add/bento/amp-image-slider' of https://github.com/rtCam…
4587ea5 
…p/amphtml into add/bento/amp-image-slider
@AnuragVasanwala
♻️ Update derived class extends using setSuperClass
89e6d54
@AnuragVasanwala
♻️ Correct dead links in markdown
5634d8b
@AnuragVasanwala
♻️ Remove @storybook/addon-knobs dependency from storybook
d8a657a
@AnuragVasanwala
♻️ Allow src/service/timer-impl.js dependency
ee9efef
@AnuragVasanwala
♻️ Update Z_INDEX.md
05fdff2
@AnuragVasanwala
Merge branch 'ampproject:main' into add/bento/amp-image-slider
9fde030
@AnuragVasanwala
♻️ Add check for initialPosition and stepSize
cc7475e
@AnuragVasanwala
Merge branch 'add/bento/amp-image-slider' of https://github.com/rtCam…
c7e3220 
…p/amphtml into add/bento/amp-image-slider
@AnuragVasanwala
🐛 Update isFiniteNumber with isNaN
e051e62
@AnuragVasanwala
Copy link
Copy Markdown
Contributor Author

AnuragVasanwala commented Apr 6, 2022 •
edited
Loading

Hi @dethstrobe 👋🏻

Would you please review this PR and provide necessary feedback.

Also, to resolve CircleCI dependency check, I have added src/service/timer-impl.js to allowList for extensions/amp-image-slider/1.0/component.js.

src/service/timer-impl.js is required for Gesture Service, otherwise calling to Gesture.get() results in following error:

Uncaught Error: Expected service timer to be registered

Please let me know if I am missing something.

Commit Ref: ee9efef

@dethstrobe dethstrobe requested a review from caroqliu April 12, 2022 15:20
@dethstrobe dethstrobe merged commit 814ca78 into ampproject:main Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dethstrobe dethstrobe dethstrobe approved these changes

@caroqliu caroqliu Awaiting requested review from caroqliu

+1 more reviewer

@deepaklalwani97 deepaklalwani97 deepaklalwani97 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AnuragVasanwala @dethstrobe @caroqliu @deepaklalwani97 @ampprojectbot @ediamin