Update: Scroll into view #906
Conversation
|
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
| if (foundItem) { | ||
| // If it is already present and visible, do nothing, but if not visible | ||
| // then scroll it into view | ||
| if (!this.isVisible(foundItem)) { |
There was a problem hiding this comment.
Is it that bad to call scrollIntoView even the thumbnail is already visible? that would allow you to get rid of isVisible and it could be kind of nice to top align the thumbnail even if it's already visible.
There was a problem hiding this comment.
it's not necessarily bad, but since cross browser support for the "smooth" scrollIntoView is not there, I was trying to avoid the slight jumpiness it causes IMO. I can show you tomorrow to see what you think
src/lib/ThumbnailsSidebar.js
Outdated
| this.renderNextThumbnailImage = this.renderNextThumbnailImage.bind(this); | ||
| this.requestThumbnailImage = this.requestThumbnailImage.bind(this); | ||
| this.thumbnailClickHandler = this.thumbnailClickHandler.bind(this); | ||
| this.toggle = this.toggle.bind(this); |
There was a problem hiding this comment.
I don't think you need to bind these
| const { scrollTop } = this.scrollingEl; | ||
| const { offsetTop } = listItemEl; | ||
|
|
||
| return scrollTop <= offsetTop && offsetTop <= scrollTop + this.containerHeight; |
There was a problem hiding this comment.
Can you put a () around scrollTop + this.containerHeight? Reading this I saw
scrollTop <= offsetTop && offsetTop <= scrollTop first 😮
| * @return {void} | ||
| */ | ||
| toggle() { | ||
| if (!this.anchorEl) { |
There was a problem hiding this comment.
Since you're already checking for anchorEl in each function below you could remove this , if you want
There was a problem hiding this comment.
I don't think I can because if there is no anchorEl, then isOpen will return false and it will attempt to invoke toggleOpen
| return; | ||
| } | ||
|
|
||
| if (!this.isOpen()) { |
There was a problem hiding this comment.
You could probably just use classList.toggle
TODO: