400% Zoom screen usability enhancements - Updated #14766
Conversation
|
Thanks for making a pull request to jupyterlab! |
|
please update galata snapshots |
|
please update documentation snapshots |
|
Documentation snapshots updated. |
|
Galata snapshots updated. |
|
@krassowski Please review and comment. |
|
Please could this PR be reviewed |
There was a problem hiding this comment.
This seems like a brilliant solution to making the UI denser at high zoom. Thank you! and apologies for taking so long to getting around to reviewing.
I reviewed all of the places where the CSS was changed and checked how the corresponding UI behaved. It looks good to me.
The only negative I could find is that this PR probably introduces some small alignment bugs at narrow width, narrow height and high zoom. (I found one having to do with filebrowser checkboxes, for example.) But I feel like these small alignment bugs can be fixed in subsequent PRs and shouldn't hold back this PR, since in my opinion it improves the usability of the UI at high zoom without seemingly degrading anything at typical zoom and viewport dimensions.
I left a couple of inline code comments.
I feel unqualified to review the following changes in this PR:
- stylelint disable directives
- snapshots
I did not carefully test this across browsers and systems. I tested carefully with Chrome on my Mac, and I looked at the changes briefly in Firefox and Safari.
packages/application/style/menus.css
Outdated
| @@ -31,7 +31,8 @@ | |||
| } | |||
|
|
|||
| .lm-MenuBar-item { | |||
| padding: 0 8px; | |||
| /* stylelint-disable-next-line csstree/validator */ | |||
| padding: 0 min(calc(1.17vw - 2px), 8px); | |||
There was a problem hiding this comment.
Just out of curiosity, why is this calc needed here? It's the only line that uses calc and I'm not exactly sure why. If I change it to min(1.17vw, 8px), it seems to work just fine.
There was a problem hiding this comment.
Hi @gabalafou the calc was added to this part of code because from the previous PR (#14626) @krassowski commented that the before changes meant the the UI was cutting off the menu on 100% zoom. So calc was added to fix this issue.
There was a problem hiding this comment.
If what this line does, specifically the - 2px part, cannot be explained, then I think it should be simplified. Or there should be some comment that explains what process was used to come up with these values.
At the moment, I really can't understand why the calc expression is needed. If you do min(1.2vw, 8px), then the screen width has to drop to 666px before anything changes—because 0.012 * 666 < 8.
Here is a screenshot showing that there's nothing wrong with the menu bar when I load this PR via GitPod and use Chrome Dev Tools to change the padding value to 0 min(1.2vw, 8px):
There was a problem hiding this comment.
Hi @gabalafou I have made the changes yo have recommend about taking out the calc from the padding. Please have a look, at the recent commit for the changes.
There was a problem hiding this comment.
Hi, I left a follow-up comment about the calc() expression. I don't think it's needed.
As I mentioned previously, it's outside of my knowledge of the repo to be able to review the snapshops or stylelint lines. We'll need to get somebody else to finish reviewing this PR.
packages/application/style/menus.css
Outdated
| @@ -31,7 +31,8 @@ | |||
| } | |||
|
|
|||
| .lm-MenuBar-item { | |||
| padding: 0 8px; | |||
| /* stylelint-disable-next-line csstree/validator */ | |||
| padding: 0 min(calc(1.17vw - 2px), 8px); | |||
There was a problem hiding this comment.
If what this line does, specifically the - 2px part, cannot be explained, then I think it should be simplified. Or there should be some comment that explains what process was used to come up with these values.
At the moment, I really can't understand why the calc expression is needed. If you do min(1.2vw, 8px), then the screen width has to drop to 666px before anything changes—because 0.012 * 666 < 8.
Here is a screenshot showing that there's nothing wrong with the menu bar when I load this PR via GitPod and use Chrome Dev Tools to change the padding value to 0 min(1.2vw, 8px):
|
please update galata snapshot |
|
please update documentation snapshots |
|
Documentation snapshots updated. |
|
@krassowski Please could you review this PR |
|
please update galata snapshot |
|
please update documentation snapshots |
|
Documentation snapshots updated. |
|
please update galata snapshots |
|
please update galata snapshots |
|
Galata snapshots updated. |
|
please update galata snapshots |
|
Galata snapshots updated. |
|
please update galata snapshots |
|
Galata snapshots updated. |
|
please update galata snapshots |
|
Galata snapshots updated. |
|
As discussed at the frontends meeting, we're not ready to merge this change in, due to its unintended side effects on UI layout and its conflicts with intermediate changes. Closing this PR; we will consider other PRs affecting #10004, particularly if they are small in scope and with little noticeable side effects. |
Opened new PR to fix merge conflict that was causing #14626 test to fail but PR ended up being closed when syncing branch.
This new PR has all the changes of #14626 plus the changes asked for by @krassowski.
References
#14626 400% Zoom screen usability enhancements
#10004 Full low Vision Support (400% Zoom) (Mobile/Tablet-Responsive Support)
Code changes
Changed padding and spacing to use min CSS function alongside vh (viewport-height) and vw(viewport-width) to reduce proportional spacing at high zoom and increase useful screen real estate usage.
User-facing changes
Currently with the 400% zoom at 1280px the help main menu bar does not appear and within the file-browser tab bar only half of one 'dir-listing' shows up.
The changes I have made now allows for the help dropdown to be shown in the main menu. I found that when trying to check the changes the sidebar 'Extension Manger' disappears from view, so this was also changes so that users that needs this implemented would be able to have access to it without zooming back out. Furthermore, within the file-browser tab, users would now be able to see least 2 notebook directory listed below than before.
Updated changes with this new PR:
The before changes meant the the UI was cutting off the menu on 100% zoom.
These are the changes made from the feedback changes the UI but at the point where it matters. These means that when returned to a 100% zoom menu back will not be cut off anymore and sidebar spacing will be fixed.
Backwards-incompatible changes
None