Skip to content
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

Make apps menu scrollable when content taller than available vertical space, preventing borking of layout #4723

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

claucambra
Copy link
Collaborator

Addresses a point in #4432

This PR:

Screen.Recording.2022-07-12.at.18.46.58.mov

Before:

Screenshot 2022-07-12 at 18 51 17

@claucambra claucambra self-assigned this Jul 12, 2022
@claucambra claucambra force-pushed the bugfix/apps-menu-height branch from f2cfdf6 to 9ab25df Compare July 12, 2022 16:54
@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #4723 (061f443) into master (fe0a26f) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4723      +/-   ##
==========================================
- Coverage   56.50%   56.40%   -0.10%     
==========================================
  Files         138      138              
  Lines       17069    17069              
==========================================
- Hits         9644     9628      -16     
- Misses       7425     7441      +16     
Impacted Files Coverage Δ
src/libsync/vfs/cfapi/hydrationjob.cpp 52.68% <0.00%> (-3.77%) ⬇️
src/libsync/vfs/cfapi/vfs_cfapi.cpp 83.84% <0.00%> (-2.63%) ⬇️
src/libsync/vfs/cfapi/cfapiwrapper.cpp 75.42% <0.00%> (-1.93%) ⬇️
src/libsync/syncengine.cpp 85.19% <0.00%> (-0.55%) ⬇️
src/libsync/propagatedownload.cpp 65.18% <0.00%> (+1.18%) ⬆️

@claucambra
Copy link
Collaborator Author

/backport to stable-3.5

@mgallien
Copy link
Collaborator

@claucambra can you ask feedback from designers ?
I am fine with the code but I do not feel comfortable approving the design myself

@nimishavijay
Copy link
Member

nimishavijay commented Jul 22, 2022

Nice! Looks much better than before! :) Only a few small suggestions:

  • We could increase the max-width of the menu to 30 or even 40% of the dialogue as some items are getting ellipsised
  • Could we add a bit of spacing to the right so that the scrollbar isn't as close to the text? Around 4px space between the end of the text and the starting of the scrollbar will probably do.
  • Move up the menu by like 2px so that it starts where the header ends (so that small gap between the header and menu is closed)

@claucambra
Copy link
Collaborator Author

Nice! Looks much better than before! :) Only a few small suggestions:

  • We could increase the max-width of the menu to 30 or even 40% of the dialogue as some items are getting ellipsised
  • Could we add a bit of spacing to the right so that the scrollbar isn't as close to the text? Around 4px space between the end of the text and the starting of the scrollbar will probably do.
  • Move up the menu by like 2px so that it starts where the header ends (so that small gap between the header and menu is closed)

Regarding the last point, this would be inconsistent with how the user menu appears:

Screenshot 2022-07-22 at 13 51 51

How would you prefer the spacing to be?

@nimishavijay
Copy link
Member

Ahh okay, good point. I see that there is a couple of pixels of space on the left of the menu as well so it's intentional :) So we can leave it be :)

@claucambra
Copy link
Collaborator Author

claucambra commented Jul 22, 2022

Ahh okay, good point. I see that there is a couple of pixels of space on the left of the menu as well so it's intentional :) So we can leave it be :)

Okay 🙂 Here's a revised version:

Screen.Recording.2022-07-22.at.14.15.05.mov

@claucambra claucambra force-pushed the bugfix/apps-menu-height branch from 9ab25df to 15c2b20 Compare July 22, 2022 12:16
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏽🚀 :)

… space, preventing borking of layout

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>
@mgallien mgallien force-pushed the bugfix/apps-menu-height branch from 15c2b20 to 061f443 Compare July 27, 2022 10:19
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4723-061f4439208de031ff5a9dec878ff9492bd49c7d-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants