feat(logos): support auto-size mode #9191

LitoMore · 2023-05-22T13:30:47Z

Resolves #8372

I will add test and documentation once we finalize the idea.

Principles

This path was initially wrapped in a viewBox="0 0 24 24" and centered. We need to reset the position to {x: 0, y: 0} of the path before updating its viewBox.

Notes

~~The parameter name logoSize and its value name cover may not be the best. Just let me know what you prefer.~~
Both load-simple-icons.js and svg-helper.js called loadSimpleIcons(), we could reorganize the code to improve its performance.

New params

logoSize
logoHeight

Previews

http://localhost:8080/badge/gaming-chip-ED1C24?logo=amd&logoColor=white&logoSize=auto
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=gray&logoSize=auto
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoHeight=10
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoWidth=20
http://localhost:8080/badge/gaming-eee?logo=amd&logoColor=black&labelColor=ED1C24&logoSize=auto
http://localhost:8080/badge/gaming-555?logo=amd&logoColor=black&labelColor=ED1C24&logoSize=auto

`pr-9191-badges-shields.fly.dev` previews

Production previews

![](https://img.shields.io/badge/gaming-chip-ED1C24?logo=amd&logoColor=white&logoSize=auto)
![](https://img.shields.io/badge/gaming-ED1C24?logo=amd&labelColor=gray&logoSize=auto)
![](https://img.shields.io/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto)
![](https://img.shields.io/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoHeight=10)
![](https://img.shields.io/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoWidth=40)
![](https://img.shields.io/badge/gaming-eee?logo=amd&logoColor=black&labelColor=ED1C24&logoSize=auto)
![](https://img.shields.io/badge/gaming-555?logo=amd&logoColor=black&labelColor=ED1C24&logoSize=auto)

github-actions · 2023-05-22T13:32:26Z

	Messages
📖	✨ Thanks for your contribution to Shields, @LitoMore!

Generated by 🚫 dangerJS against b22cfdb

socket-security · 2023-05-22T13:32:41Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

chris48s · 2023-05-23T19:51:50Z

Thanks for having a look at this.

@calebcartwright - in terms of the concept (i.e: "do we think this feature is a good idea?", as opposed to "is this good code?"), what are your views on this one?

There is some more detail/discussion in #8372

calebcartwright · 2023-06-04T18:45:02Z

@calebcartwright - in terms of the concept (i.e: "do we think this feature is a good idea?", as opposed to "is this good code?"), what are your views on this one?

I agree that the auto-sizing feature is a good idea. I haven't looked at the implementation at all so not sure what the current state on this topic is, but my only preference is that this behavior be an explicit opt-in (e.g. via a query param). I say that because it feels like this is relevant in more niche use cases and I don't want to apply it by default as I suspect that would be accompanied with a large number of unknowns

chris48s · 2023-06-04T18:47:31Z

OK, sounds reasonable. I'll put this one on my list to review the code then

chris48s · 2023-06-18T19:20:02Z

I've had a look over this. Broadly, the implementation looks fine. I'll leave a few comments inline.

We would need to add some documentation for this param. In the new frontend, globally available params are documented in

shields/core/base-service/openapi.js

Line 2 in e7737ce

const globalParamRefs = [
shields/core/base-service/openapi.js

Line 233 in e7737ce

parameters: {

Other admin: there are failing tests and merge conflicts that would need to be sorted.

chris48s · 2023-06-18T19:20:12Z

In terms of naming, I'm slightly unsure what the best name for this param is either, but I'm keen to get this right as once it is out there, we're basically stuck with it forever.

Some other viable suggestions are ?logoSize=auto or ?logoSize=fit. Naming things is hard.

Any other maintainers have a view on this?

core/base-service/coalesce-badge.js

calebcartwright · 2023-06-18T19:25:24Z

Any other maintainers have a view on this?

I've a mild preference for ?logoSize=auto

socket-security · 2023-06-19T08:29:42Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher

View full report↗︎

github-actions · 2023-06-19T14:33:31Z

🚀 Updated review app: https://pr-9191-badges-shields.fly.dev

github-actions · 2023-06-22T17:45:23Z

🚀 Updated review app: https://pr-9191-badges-shields.fly.dev

github-actions · 2023-06-23T18:32:52Z

🚀 Updated review app: https://pr-9191-badges-shields.fly.dev

calebcartwright · 2023-08-12T21:47:31Z

Recognize this has some conflicts that would be blockers, but in general, where did we land as far as our target strategic direction with this one?

chris48s · 2023-08-13T15:17:10Z

where did we land as far as our target strategic direction with this one?

My last comment was

add the logoSize=auto param as a first step and then see where we think we should go next based on user feedback. Adding that doesn't prevent us adding more params later. Once we have added params or settings, removing or changing them is harder: We can't really version a service like we can with a library.

and P-Y 👍d it You good with that?

calebcartwright · 2023-08-13T18:37:31Z

where did we land as far as our target strategic direction with this one?

My last comment was

add the logoSize=auto param as a first step and then see where we think we should go next based on user feedback. Adding that doesn't prevent us adding more params later. Once we have added params or settings, removing or changing them is harder: We can't really version a service like we can with a library.

and P-Y +1d it You good with that?

Ah yes, it's coming back to me now. This was one you and I discussed synchronously on the team call and I'd intended to comment indicating my 👍 but forgot to do so. Apologies for the noise, but yes indeed, I'm 👍 with that

LitoMore · 2023-08-27T01:57:10Z

I've resolved conflicts and rebased with the latest code.

calebcartwright · 2023-09-14T00:04:23Z

Seems CI is still rather unhappy. One is just the linter, and a quick npm run prettier should fix, but the others failing jobs have test failures, e.g.:

https://github.com/badges/shields/actions/runs/6156362420?pr=9191

AssertionError: expected { openapi: '3.0.0', …(3) } to deeply equal { openapi: '3.0.0', …(3) }
at Context. (file:///home/runner/work/shields/shields/core/base-service/openapi.spec.js:411:15)
at process.processImmediate (node:internal/timers:476:21)

chris48s · 2023-09-17T08:24:03Z

I checked this out locally. I'm seeing that logoHeight is still exposed as a URL param
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoHeight=1
http://localhost:8080/badge/gaming-ED1C24?logo=amd&labelColor=555&logoSize=auto&logoHeight=100

LitoMore · 2023-09-20T07:42:20Z

@chris48s The exposed logoHeight should be removed now.

github-actions · 2023-09-24T11:45:43Z

🚀 Updated review app: https://pr-9191-badges-shields.fly.dev

chris48s

Thanks. I think we've now got the behaviour to expose settled.

I've now had a proper look over the code and flagged some points we'll need to get settled before we can merge. It is mostly quite minor stuff now though.

lib/logos.js

core/base-service/openapi.js

services/endpoint/endpoint.service.js

core/base-service/openapi.js

lib/logos.js

github-actions · 2024-04-22T11:30:33Z

🚀 Updated review app: https://pr-9191-badges-shields.fly.dev

chris48s

Thanks for following up - latest changes LGTM. I've pushed another commit just resolving the merge conflicts on package.json and package-lock.json

chris48s · 2024-04-22T14:22:18Z

Quick FYI.
I deployed this to production today, but hit a bit of an issue. Its unrelated to this PR, but basically this happened #10077 but in production this time.

There's nothing you can do to help - its an issue with fly.io - but in order to get things stable I had to roll back to the previous release. I'll try deploying again later in the week, but just wanted to clarify what happened because this was deployed for a bit and then rolled back.

The JSR logo has been added to Simple Icons since simple-icons/simple-icons#10794. I used the `logoSize` parameter to make the logo fit the badge. The feature was introduced at badges/shields#9191. Currently, we can't use `logoSize` in the response due to badges/shields#10132. So I passed these parameters with search parameters instead.

LitoMore changed the title ~~feat(logos): support cover mode~~ feat(logos): support auto-size mode May 22, 2023

chris48s self-requested a review June 4, 2023 18:47

chris48s reviewed Jun 18, 2023

View reviewed changes

core/base-service/coalesce-badge.js Show resolved Hide resolved

core/base-service/coalesce-badge.js Outdated Show resolved Hide resolved

LitoMore force-pushed the logo-cover-mode branch from b70a15d to bc29d87 Compare June 19, 2023 08:27

LitoMore force-pushed the logo-cover-mode branch 2 times, most recently from d544d98 to b03a4c1 Compare June 19, 2023 09:45

LitoMore marked this pull request as ready for review June 19, 2023 09:45

LitoMore requested review from chris48s and calebcartwright June 19, 2023 09:47

LitoMore force-pushed the logo-cover-mode branch from b03a4c1 to 0cb2518 Compare June 25, 2023 15:51

LitoMore mentioned this pull request Aug 7, 2023

Update Zoom icon simple-icons/simple-icons#9211

Merged

LitoMore force-pushed the logo-cover-mode branch from 0cb2518 to d83ad1b Compare August 27, 2023 01:56

LitoMore requested a review from PyvesB August 27, 2023 02:02

chris48s reviewed Sep 24, 2023

View reviewed changes

LitoMore force-pushed the logo-cover-mode branch 4 times, most recently from d4fafe3 to 9eaaddb Compare April 3, 2024 18:45

feat(logos): support auto-sizing mode

bafc19d

LitoMore force-pushed the logo-cover-mode branch from 9eaaddb to bafc19d Compare April 3, 2024 18:47

LitoMore requested a review from chris48s April 3, 2024 18:49

LitoMore mentioned this pull request Apr 10, 2024

Add JSR icon simple-icons/simple-icons#10794

Merged

3 tasks

Merge branch 'master' into logo-cover-mode

b22cfdb

chris48s approved these changes Apr 22, 2024

View reviewed changes

chris48s enabled auto-merge April 22, 2024 11:43

chris48s added this pull request to the merge queue Apr 22, 2024

Merged via the queue into badges:master with commit 915ab74 Apr 22, 2024
21 checks passed

LitoMore deleted the logo-cover-mode branch April 22, 2024 11:52

chris48s added the core Server, BaseService, GitHub auth label Apr 22, 2024

This was referenced Apr 22, 2024

add logoSize to globalQueryParams #10110

Merged

static badge bypasses globalQueryParams #10111

Open

chris48s mentioned this pull request Apr 26, 2024

improve performance of getIconSize() #10125

Merged

LitoMore mentioned this pull request Apr 28, 2024

Polish XO badge xojs/xo#750

Merged

LitoMore mentioned this pull request May 7, 2024

refactor: use shields.io built-in JSR logo for badges jsr-io/jsr#487

Merged

LitoMore mentioned this pull request May 18, 2024

Now you're be able to use logoSize=auto alexandresanlim/Badges4-README.md-Profile#658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logos): support auto-size mode #9191

feat(logos): support auto-size mode #9191

LitoMore commented May 22, 2023 •

edited

github-actions bot commented May 22, 2023 •

edited

socket-security bot commented May 22, 2023 •

edited

chris48s commented May 23, 2023 •

edited

calebcartwright commented Jun 4, 2023

chris48s commented Jun 4, 2023

chris48s commented Jun 18, 2023

chris48s commented Jun 18, 2023

calebcartwright commented Jun 18, 2023

socket-security bot commented Jun 19, 2023 •

edited

github-actions bot commented Jun 19, 2023

github-actions bot commented Jun 22, 2023

github-actions bot commented Jun 23, 2023

calebcartwright commented Aug 12, 2023

chris48s commented Aug 13, 2023

calebcartwright commented Aug 13, 2023

LitoMore commented Aug 27, 2023

calebcartwright commented Sep 14, 2023

chris48s commented Sep 17, 2023

LitoMore commented Sep 20, 2023

github-actions bot commented Sep 24, 2023

chris48s left a comment

github-actions bot commented Apr 22, 2024

chris48s left a comment

chris48s commented Apr 22, 2024

feat(logos): support auto-size mode #9191

feat(logos): support auto-size mode #9191

Conversation

LitoMore commented May 22, 2023 • edited

Principles

Notes

New params

Previews

pr-9191-badges-shields.fly.dev previews

Production previews

github-actions bot commented May 22, 2023 • edited

socket-security bot commented May 22, 2023 • edited

chris48s commented May 23, 2023 • edited

calebcartwright commented Jun 4, 2023

chris48s commented Jun 4, 2023

chris48s commented Jun 18, 2023

chris48s commented Jun 18, 2023

calebcartwright commented Jun 18, 2023

socket-security bot commented Jun 19, 2023 • edited

github-actions bot commented Jun 19, 2023

github-actions bot commented Jun 22, 2023

github-actions bot commented Jun 23, 2023

calebcartwright commented Aug 12, 2023

chris48s commented Aug 13, 2023

calebcartwright commented Aug 13, 2023

LitoMore commented Aug 27, 2023

calebcartwright commented Sep 14, 2023

chris48s commented Sep 17, 2023

LitoMore commented Sep 20, 2023

github-actions bot commented Sep 24, 2023

chris48s left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024

chris48s left a comment

Choose a reason for hiding this comment

chris48s commented Apr 22, 2024

LitoMore commented May 22, 2023 •

edited

`pr-9191-badges-shields.fly.dev` previews

github-actions bot commented May 22, 2023 •

edited

socket-security bot commented May 22, 2023 •

edited

chris48s commented May 23, 2023 •

edited

socket-security bot commented Jun 19, 2023 •

edited