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

Fix size of icons in menus inside apps when shown as images #11054

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Sep 4, 2018

This pull request fixes the icons in the contacts menu popover for Files app, although it can potentially fix other menus too.

In our beloved #9982 :-P the descendants of .app-* elements were set to use border-box sizing. This caused the descendants of the Files app (like the comments and the sharing tab in its sidebar) to use border-box sizing (before they used content-box because there was no ancestor #app element).

The main CSS rules for images in popover menus assume that content-box sizing is used, so when they were shown using border-box they were not properly shown. Now both the width and height of the image is set to the item height in that case, which causes the visible size of the icon to be the item height minus the padding (the same as when content-box sizing is used).

Before:
contactsmenupopover-icons-before

After:
contactsmenupopover-icons-after

@nextcloud/designers

Some popover menus, like the contacts menu, still show their icon using
an img element. The main CSS rules assume that a "content-box" sizing is
being used, and thus set the size and padding of the image to add up to
the line height.

However, ".app-*" descendants use a "border-box" sizing, so when a menu
with an image was shown in an app the icon was not properly shown. Now
both the width and height of the image is set to the item height in
those cases, which causes the visible size of the icon to be the item
height minus the padding (the same as when "content-box" sizing is
used).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@skjnldsv
Copy link
Member

skjnldsv commented Sep 5, 2018

I added this css border-box because it was already in. But I see no points in keeping it if you want.
I'm not fan of dedicated fixing rules like those, i would prefer fixing the issue at the root of the problem :(

Nice catch on the other hand!! 🤗

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🐘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc. regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants