Skip to content

Allow packages with no logo #171

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow packages with no logo #171

wants to merge 1 commit into from

Conversation

gtca
Copy link
Contributor

@gtca gtca commented Jul 15, 2025

no_icon = true can now be added to the package information to render them appropriately on the main page.

Package names can also be capitalized now as their names will be converted to lowercase to fetch the correct icon files based on the package name.
Though I think all-lowercase is something we should keep, maybe in the case of snapatac2 vs SnapATAC2 it is a bit too much.

Packages names can also be capitalized now as their names
will be converted to lowercase to fetch the correct icon files
based on the package name.
Copy link

netlify bot commented Jul 15, 2025

Deploy Preview for jade-cajeta-1bcca0 ready!

Name Link
🔨 Latest commit c1a26ee
🔍 Latest deploy log https://app.netlify.com/projects/jade-cajeta-1bcca0/deploys/6876aae51cabad0008cd9a1e
😎 Deploy Preview https://deploy-preview-171--jade-cajeta-1bcca0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@gtca gtca requested a review from Zethson July 15, 2025 19:25
@maltekuehl
Copy link
Collaborator

Citing from the Zen of Python, even though this is a different language: "Explicit is better than implicit". I am not sure that having a no_icon is the best solution, as it still hides where the icons for the other packages come from. Imo, the code would be more explicit and understandable if instead icons (e.g., their paths, if that is not possible, a flag) were always defined. Then the no icon case could become icon = null, with a shared attribute for all packages and no "magic" behind the scences.

@maltekuehl
Copy link
Collaborator

This would also make for a more flexible setup. E.g., with the current code with {lower .name}, what would happen if the name itself contains a forward slash? Ofc, impact is likely low in this very limited context in this small website package, just if you are changing it already might be something worth considering.

Copy link
Member

@Zethson Zethson left a comment

Choose a reason for hiding this comment

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

Thank you! I would have sworn that I had used the scverse logo also on the main page for SnapATAC2. But uhm apparently not?

If we don't show a logo, we have an inconsistency because on https://scverse.org/packages/#core-packages we show the scverse logo for SnapATAC2. I had wanted to do the same for the main page.
I don't have a strong opinion on this but I totally trust Malte and would encourage you to consider what he says.

Thank you @gtca !

@gtca
Copy link
Contributor Author

gtca commented Jul 15, 2025

I think explicit logo paths is the way to go, and it is the thing for the packages page. (It is an implicit link to the filename pkgname.svg on the main page.)

Thanks both for your comments!

@flying-sheep
Copy link
Member

flying-sheep commented Jul 16, 2025

I like when there’s one source of truth and it’s impossible to e.g. both specify no_icon and have the icon file exist.

But I also think that the presence of a file is an explicit thing, not an implicit one.

If we want locality, we could maybe have one folder per package with both logo and metadata inside, and deleting or adding a folder would remove or add a package. But idk if Hugo supports globbing like that.

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.

4 participants