3873: feat configure number of thumbnails #3915
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. |
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
| @@ -512,6 +512,8 @@ export default { | |||
| preferredFormats: ['jpg', 'png', 'webp', 'tif'], | |||
| }, | |||
| 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 :)
Hi,
i created a PR which solves issue 3873