3873: feat configure number of thumbnails #3915
3873: feat configure number of thumbnails #3915fstoe wants to merge 5 commits intoProjectMirador:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3915 +/- ##
=======================================
Coverage 94.52% 94.52%
=======================================
Files 313 313
Lines 14767 14781 +14
Branches 2496 2501 +5
=======================================
+ Hits 13958 13972 +14
Misses 804 804
Partials 5 5 ☔ View full report in Codecov by Sentry. |
lutzhelm
left a comment
There was a problem hiding this comment.
The changes look like they work as intended, but I have a strange feeling about the usability. Maybe it would be better to center the thumbnail navigation if there is a limit applied. If there were only five canvases in the manifest, the thumbnails would also align left (or right for RTL). But aligning a component with a scrollbar to the left feels strange.
I guess I'll try to get some feedback in this weeks community call.
src/config/settings.js
Outdated
| }, | ||
| thumbnailNavigation: { | ||
| count: 5, // The amount of thumbnails to be shown | ||
| limit: false, // Limits the shown thumbnails in the thumbnail navigation |
There was a problem hiding this comment.
- Couldn't these two settings be reduced to a single setting? Something like, the
countsetting does only apply if it is >0. I think we should avoid cluttering the settings with too many options. In that case, the setting should be set to0as default. - Is there a more precise name than
count(or limit, for that matter)? LikemaxNumberOfThumbnails, or something preferably shorter? - Alphabetical order would be nice to make eslint happy.
There was a problem hiding this comment.
Thanks. I made the requested changes. Let me know if i should center the navigation or not :)
|
@lutzhelm is there a way we might have it fetch enough thumbnails to fill the window, and then push/pop canvases with a loaded thumbnail on scroll? |

Hi,
i created a PR which solves issue 3873