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

Change background style target for tab icons #2374

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 29, 2021

The style was applied to the containing element instead of the icon element.

Don't know why the background-position: center 8px; was needed, so I removed it. It was also shifting icons toward the bottom when applied to the icon.

Tested in styleguide.

Fix: nextcloud/activity#669
Problem introduced here: #1939

@artonge artonge marked this pull request as draft November 29, 2021 12:49
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the fix/sidebar_tabs_icon_rendering branch from 35650b4 to 8f3b4cf Compare November 29, 2021 14:17
@artonge artonge added 2. developing Work in progress bug Something isn't working labels Nov 29, 2021
@artonge artonge self-assigned this Nov 29, 2021
@artonge artonge marked this pull request as ready for review November 29, 2021 14:20
@artonge artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 29, 2021
@skjnldsv
Copy link
Contributor

Don't know why the background-position: center 8px; was needed,

To support the legacy icon methods iirc. But that changed when we added compatibility for the MaterialDesignIcons

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Nov 29, 2021

I am a bit puzzled why this only happens for the activity icon. All other icons in the files sidebar of c.nc.c are fine, although they also use the legacy icons and not material design.

But your changes fix the problem, so 👍

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Nov 29, 2021

I am a bit puzzled why this only happens for the activity icon. All other icon in the files sidebar of c.nc.c are fine, although they also use the legacy icons and not material design.

I found out what causes this. The activity icon specifies a width, height and viewBox of 32 px in the svg:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg xmlns="http://www.w3.org/2000/svg" height="32" width="32" version="1.0" viewBox="0 0 32 32">
 <path d="m16 1-10 18h11l-1 12 10-18h-11z" fill="#FFF"/>
</svg>

whereas all other icons only specify the (correct) size of 16px, e.g. spreed:

<svg width="16" height="16" version="1.1" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
<path d="m7.9992 0.999a6.9993 6.9994 0 0 0-6.9992 6.9996 6.9993 6.9994 0 0 0 6.9992 6.9994 6.9993 6.9994 0 0 0 3.6308-1.024c0.86024 0.34184 2.7871 1.356 3.2457 0.91794 0.47922-0.45765-0.56261-2.6116-0.81238-3.412a6.9993 6.9994 0 0 0 0.935-3.4814 6.9993 6.9994 0 0 0-6.9991-6.9993zm8e-4 2.6611a4.34 4.3401 0 0 1 4.34 4.3401 4.34 4.3401 0 0 1-4.34 4.3398 4.34 4.3401 0 0 1-4.34-4.3398 4.34 4.3401 0 0 1 4.34-4.3401z" fill="#fff" stroke-width=".14"/>
</svg>

So for the larger activity icon we have to explicitly set the size correctly.

@artonge artonge requested review from Pytal and CarlSchwan November 30, 2021 09:20
@artonge artonge merged commit c6adde6 into master Nov 30, 2021
@artonge artonge deleted the fix/sidebar_tabs_icon_rendering branch November 30, 2021 09:37
@raimund-schluessler
Copy link
Contributor

/backport to stable4

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 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activity sidebar tab icon is broken in 23 and master
4 participants