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

Add auto-size support for Simple Icons logos #8372

Closed
LitoMore opened this issue Sep 2, 2022 · 8 comments · Fixed by #9191
Closed

Add auto-size support for Simple Icons logos #8372

LitoMore opened this issue Sep 2, 2022 · 8 comments · Fixed by #9191

Comments

@LitoMore
Copy link
Contributor

LitoMore commented Sep 2, 2022

📋 Description

The logo feature only renders in the viewBox="0 0 24 24". For some logos, we may not get a nice view. For example:


We could calculate the aspect ratio of the icon by using svg-path-bbox:

import svgPathBbox from 'svg-path-bbox'

const [x0, y0, x1, y1] = svgPathBbox(icon.path)
const width = x1 - x0
const height = y1 - y0

// We may need to deal with `width > height` for badges
if (width > height) {
  // Resize the logo part
}

We could handle the resizing logic in load-simple-icons.js or other places.

Other logoWidth and logoHeight related logics may also need to be updated.

@LitoMore LitoMore changed the title Add auto-width support for Simple Icons logos Add auto-size support for Simple Icons logos Sep 2, 2022
@chris48s
Copy link
Member

chris48s commented Sep 4, 2022

Just to summarise: At the moment, the logo portion of the badge is a fixed width. Your suggestion is that the logo portion of the badge becomes variable width - is that right?

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 4, 2022

Just to summarise: At the moment, the logo portion of the badge is a fixed width. Your suggestion is that the logo portion of the badge becomes variable width - is that right?

Yes.

@chris48s
Copy link
Member

chris48s commented Sep 4, 2022

Got it.

So I guess the tradeoff we have now is there is some subset of "wide but short" logos that render unhelpfully small, whereas one of the nice things about the way we do things right now is that logos which don't take up 100% of the viewport vertically but look perfectly fine are all the same width. e.g: These 3 logos all have slightly different non-blank vertical height, but they all render the same width:

Going to a variable horizontal width for logos would make all of those different widths.

Then thinking about the most annoying edge case I can generate in the other direction: Imagine a "logo" which is a 1 pixel wide horizontal line. That would cause us to render a really really wide badge :) Obviously that's a deliberately adversarial input, but worth thinking about.

I'm on the fence about this - it might be good to mock up some example images to help decide. Maybe if we accepted a PR for this it would make sense to set some kind of upper and lower bound on this. For example:

  • If the logo fills more than X% of the vertical space in the viewport, we don't resize it
  • There is an upper limit on how much horizontal space on the badge the logo can take up

Between those thresholds, the width can vary to try and better accommodate "wide but short" logos within reasonable parameters.

I also wonder if this would make sense to apply only to SimpleIcons only, or also to custom user-supplied logos..

@chris48s
Copy link
Member

chris48s commented Sep 4, 2022

Another thought: Would vertical alignment be considered relevant. i.e: Would these 2 images be treated as the same or different when it comes to ignoring vertical whitespace:

Screenshot at 2022-09-04 21-48-26

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 5, 2022

We should treat them the same.

We could always align the icon to a vertical & horizontal center.

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 5, 2022

  • If the logo fills more than X% of the vertical space in the viewport, we don't resize it
  • There is an upper limit on how much horizontal space on the badge the logo can take up

Or just let users choose when to resize. We could provide a new parameter like &logoSize=fit.

The fit is just a proposal, or we could refer to the CSS background-size , name it to contain or cover.

  • contain: Current behavior what we deal with the icon
  • cover: Resize the icon

I also wonder if this would make sense to apply only to SimpleIcons only, or also to custom user-supplied logos.

Should apply to Simple Icons and customs logos, IMO.

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 5, 2022

I will post more examples of different size icons later.

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 8, 2022

Example:

CleanShot 2022-09-08 at 22 53 00@2x

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 a pull request may close this issue.

2 participants