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.

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

Lookup event emitter on scroll #253

Merged
merged 2 commits into from Nov 9, 2020
Merged

Lookup event emitter on scroll #253

merged 2 commits into from Nov 9, 2020

Conversation

guergana
Copy link
Contributor

@guergana guergana commented Nov 3, 2020

  • an event is emitted from Lookup when user scrolls in the LookupMenu that contains the first and last indexes of visible elements
  • added a browser test

@guergana
Copy link
Contributor Author

guergana commented Nov 3, 2020

I am aware that the scroll event might need to be throttled. I am not sure about what is the best approach for doing this in vue. Should I use a setTimeout, is 300 a good delay? Making the event passive already improves the performance.

@itamargiv
Copy link
Member

itamargiv commented Nov 4, 2020 •
edited

I am aware that the scroll event might need to be throttled.

That's great thinking! IMO maybe a debounce would work even better for this kind of task (for anyone interested in more context, see: https://redd.one/blog/debounce-vs-throttle).

I am not sure about what is the best approach for doing this in vue.

Whichever you choose, it would probably be best to make the chosen function reusable, or even rely on an external lib like lodash (I'm more of a less deps better approach, but it's really a personal choice in this case, as long as the library is maintained and well supported) see this stack overflow thread for deets.

itamargiv
itamargiv requested changes Nov 4, 2020
View reviewed changes
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

Blocking until implementation of throttle / debounce.

@micgro42
Copy link
Collaborator

micgro42 commented Nov 4, 2020

I think we used a debounce in data bridge. I'll look it up

@micgro42
Copy link
Collaborator

micgro42 commented Nov 4, 2020

Here we are using lodash's debounce:

https://github.com/wikimedia/wikibase-vuejs-components/blob/master/src/components/ResizingTextField.vue

@guergana
Copy link
Contributor Author

guergana commented Nov 4, 2020

Will use debounce then. Thanks for the input @micgro42 and @itamargiv

@guergana guergana force-pushed the lookup-scroll branch 3 times, most recently from 9d6370b to 16466af Compare November 4, 2020 16:41
itamargiv
itamargiv requested changes Nov 5, 2020
View reviewed changes
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

A few minor changes and we might be good to go. Looking great otherwise!

vue-components/src/components/Lookup.vue Outdated Show resolved Hide resolved
vue-components/stories/Lookup.stories.ts Show resolved Hide resolved
vue-components/src/components/Lookup.vue Show resolved Hide resolved
tzhelyazkova
tzhelyazkova suggested changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@tzhelyazkova tzhelyazkova left a comment

Choose a reason for hiding this comment

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

Requesting changes because of the ref vs querySelectorAll comment.

vue-components/src/components/LookupMenu.vue Show resolved Hide resolved
vue-components/src/components/LookupMenu.vue Outdated Show resolved Hide resolved
tzhelyazkova
tzhelyazkova suggested changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@tzhelyazkova tzhelyazkova left a comment

Choose a reason for hiding this comment

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

Requesting changes, because of the ref business. I think the PR is almost there 👍

vue-components/src/components/LookupMenu.vue Show resolved Hide resolved
vue-components/src/components/LookupMenu.vue Outdated Show resolved Hide resolved
@guergana guergana force-pushed the lookup-scroll branch 2 times, most recently from a53ed72 to 2c62163 Compare November 9, 2020 12:57
itamargiv
itamargiv previously approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

Looking like it's going well, I will approve on my side, but agree with @tzhelyazkova that we should use refs where possible.

tzhelyazkova
tzhelyazkova previously approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@tzhelyazkova tzhelyazkova left a comment

Choose a reason for hiding this comment

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

Approving since the ref thing was addressed.

itamargiv
itamargiv requested changes Nov 9, 2020
View reviewed changes
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

:( Sorry, it really is good to go, it's really just that small docs thing I approved too soon

vue-components/src/components/Lookup.vue Outdated Show resolved Hide resolved
@guergana
Lookup event emitter on scroll
f460ec1 
+ an event is emitted from Lookup when user scrolls
in the LookupMenu that contains the first and last
indexes of visible elements
+ added a browser test
@itamargiv
Copy link
Member

🎊 Thank you, great work!! 🎊
Mr. T holding a Torah Scroll up and exclaiming: "Let's Scroll!"

@guergana guergana merged commit 28302a3 into master Nov 9, 2020
@guergana guergana deleted the lookup-scroll branch November 9, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobw jakobw jakobw left review comments

@itamargiv itamargiv itamargiv approved these changes

@tzhelyazkova tzhelyazkova tzhelyazkova left review comments

@Ladsgroup Ladsgroup Awaiting requested review from Ladsgroup

@micgro42 micgro42 Awaiting requested review from micgro42

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@guergana @itamargiv @micgro42 @jakobw @tzhelyazkova