Now using dynamic percentual appmenu limit #5244
Conversation
Signed-off-by: Patrik Kernstock <info@pkern.at>
|
@patschi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @icewind1991 and @nickvergessen to be potential reviewers. |
|
@nextcloud/designers let’s review! :) @karlitschek what do you think about this approach? |
|
nice 👍 |
|
@juliushaertl @eppfel @MorrisJobke @pixelipo what do you think? :) |
core/js/js.js
Outdated
| @@ -1512,12 +1512,17 @@ function initCore() { | |||
|
|
|||
| var resizeMenu = function() { | |||
| var appList = $('#appmenu li'); | |||
| var availableWidth = $('#header-left').width() - $('#nextcloud').width() - 44; | |||
| var usePercentualAppMenuLimit = 33; | |||
| var availableWidth = (($('#header-left').width() - $('#nextcloud').width()) / 100 * usePercentualAppMenuLimit); | |||
There was a problem hiding this comment.
Why not just multiply by 0.33? It would get the same result and decimal is an acceptable representation for percentage.
There was a problem hiding this comment.
Well, it's just a personal preference. I'm fine with decimal notation as well.
core/js/js.js
Outdated
| @@ -1512,12 +1512,17 @@ function initCore() { | |||
|
|
|||
| var resizeMenu = function() { | |||
| var appList = $('#appmenu li'); | |||
| var availableWidth = $('#header-left').width() - $('#nextcloud').width() - 44; | |||
| var usePercentualAppMenuLimit = 33; | |||
| var availableWidth = (($('#header-left').width() - $('#nextcloud').width()) / 100 * usePercentualAppMenuLimit); | |||
| var appCount = Math.floor((availableWidth)/44); | |||
There was a problem hiding this comment.
I'm not a big fan of hardcoded values. Since we are already fetching logo and app-menu widths from DOM, why not fetch single-app-width as well? It would save us one headache if/when Nextcloud gets redesigned...
There was a problem hiding this comment.
Tested fine using $(appList).width(). Or any other prefered solution from your side?
Yup, let's leave it at 33% because we also discussed different values before. :) Your other comments are good! |
Signed-off-by: Patrik Kernstock <info@pkern.at>
Codecov Report
@@ Coverage Diff @@
## master #5244 +/- ##
============================================
+ Coverage 54.14% 54.15% +<.01%
Complexity 22314 22314
============================================
Files 1381 1381
Lines 85472 85484 +12
Branches 1325 1329 +4
============================================
+ Hits 46283 46294 +11
- Misses 39189 39190 +1
|
|
I'm not that happy with the 33%. I usually have my browser on one half of the screen and that way only 4 icons are visible. Not sure if most people have their browser on full screen, but for picking a higher percentage number would make more sense.
|
|
Right, also just saw that. I agree and it's different than we specified. Because here it's in desktop mode (more than 768px width) and hence at least the 8 icons (or 7 + menu) should be shown.
Ok? |
|
@eppfel @jancborchardt I've pushed a commit to fix that. Please review.
|
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr
force-pushed
the
dynamic-percentual-appmenu-limit
branch
from
7a8e763 to
cec2893
Compare
|
Why isn't there a Nextcloud administrator option to change a hard-coded default 33% etc.? To me it makes no sense whatsoever to present a 60% Blank Tool Bar to users and hide 11 active tool icons in the ... additional tool app bin forcing users to hunt and peck for them at best. Users want to see active tools at a mere glance, Nextcloud is now hiding a majority of the active tools in its tool overflow bin ... to keep the top tool bar Empty Space which makes no sense whatsoever. Let the individual administrators set and determine the % of empty tool bar space if need be on this. |
PR for using percentual appmenu limit like discussed in #5135. Further discussions in the linked issue.