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
Thumbnail scrolling #207
Thumbnail scrolling #207
Conversation
I suspect that dropping IE11 (or not) should be a clear, conscious decision given that we often choose to support old stuff. |
Basic testing:
|
Testing in Chrome
This works fine Multiple selected images > Open with iviewer. Url like /iviewer/?images=4402,4403,4404,4405 If I select all the images in the dataset All the thumbnails are loaded |
I don't think we can drop IE |
Noticeable loading difference between FF and chrome |
FF or Vivaldi This time |
I too saw that "Not Found": https://openmicroscopy.slack.com/archives/C0K5EED3R/p1535619171000100 |
kymograph is correctly displayed cc @pwalczysko |
While testing 5.4.8 |
@jburel Re: "If I select all the images in the dataset All the thumbnails are loaded |
All the thumbnails were loaded even the non visible when I do a selection of images |
Also noticed that e.g. http://web-dev-merge.openmicroscopy.org/iviewer/?well=339085 (user-1) doesn't work (no thumbnails or image loaded). |
Re: #207 (comment) The 'Not Found' in this case is the same as previously (not related to this PR). To fix that we could use /webclient/render_thumbnail/ID instead of /webgateway/render_thumbnail. Then we'd get the 'No Image' icon same as webclient? |
Fixed the placeholder "Not Found" while images loading in Firefox. |
src/app/thumbnail-slider.js
Outdated
count = 0; | ||
} | ||
this.thumbnails.length = count; | ||
this.thumbnails.fill({'version': 0, 'title': 'unloaded'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given #207 (comment) is this okay?
No cover yet: IE testing |
src/app/thumbnail-slider.js
Outdated
|
||
/** | ||
* the end index of the displayed thumbnails | ||
* height of slider, so we which thumbs have scrolled into view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing verb
src/app/thumbnail-slider.js
Outdated
|
||
/** | ||
* @constructor | ||
* @param {Context} context the application context (injected) | ||
* @param {BindingEngine} bindingEngine the BindingEngine (injected) | ||
*/ | ||
constructor(context, element, bindingEngine) { | ||
constructor(context, element, bindingEngine, taskQueue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare new parameter
/** | ||
* Loads unloaded thumbnails that scrolled into the thumbnail panel | ||
* | ||
* @param {number} scrollTop Current scroll position of the panel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare parameter
src/app/thumbnail-slider.js
Outdated
* Adds thumbnails to then be loaded. | ||
* Adds thumbnails with Image IDs to this.thumbnails array. | ||
* We create a new list from old, replacing the thumbnails (instead of | ||
* modifying the old list) to make sure the changes is observed and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is/are
src/app/thumbnail-slider.js
Outdated
/** | ||
* Adds thumbnails with Image IDs to this.thumbnails array. | ||
* We create a new list from old, replacing the thumbnails (instead of | ||
* modifying the old list) to make sure the changes is observed and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is/are
src/app/thumbnail-slider.js
Outdated
* modifying the old list) to make sure the changes is observed and | ||
* UI updates. | ||
* | ||
* @param {Array.<Object>} thumbnails the thumbnails to be added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thumbnails not a parameter
Testing in IE 11 |
See https://trello.com/c/JLpfTbmW/4-improve-thumbnails-loading-display
Instead of clicking up/down to load more thumbnails, the thumbnail panel is created at the full size (with only the visible thumbnails loaded) and more thumbnails are loaded when we scroll.
There are lots of different cases to handle and a lot has changed so everything needs testing:
Scenarios to test:
/webclient/img_detail/4413/?dataset=1301
- Check that thumbs loaded from current parent Dataset (not a different Dataset that the image is in)
- Try double-clicking images in webclient near the start of the Dataset and at the end. Thumb panel in iviewer should scroll to the show the selected image.
- Try with a small Dataset (< 5 images) and as large as possible.
- Remove the
?datasets=ID
from the URL and reload. If image is in single Dataset, thumbs should be loaded. If more than 1 Dataset, thumbnail panel hidden./iviewer/?images=4402,4403,4404,4405
webclient/img_detail/57012/
should hide thumbnail panel./iviewer/?well=148
First image should be selected./iviewer/?dataset=10480
First image selected.