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

detail-view: Add extension icon #494

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

oscfdezdz
Copy link
Collaborator

@oscfdezdz oscfdezdz commented Oct 15, 2023

If the extension has one.

For now it is mostly a duplication of the code that manages screenshots.

image

@oscfdezdz oscfdezdz marked this pull request as draft October 15, 2023 19:58
@oscfdezdz
Copy link
Collaborator Author

In those without icon, would it be better to show a symbolic or not show anything at all? From what I have read of the default icon, it was one of the problems stated in the GNOME Circle review. This change could be discussed later, after the requirements are met.

@mjakeman
Copy link
Owner

Having a think about this - from a code perspective, looks good 👍

The main worry is that extension icons are very low-res. We'd probably want to avoid displaying pixellated or blurry icons, so it might be worth giving a maximum size (say 64x64) and always rendering at that resolution. I think if we do this, it'd be worth having a placeholder icon too. As per the Circle review, I imagine they'd want a symbolic.

@oscfdezdz oscfdezdz force-pushed the oscfdezdz/extension-icon branch 2 times, most recently from 59b9a90 to ff31b6e Compare November 20, 2023 11:10
@oscfdezdz
Copy link
Collaborator Author

Using puzzle symbolic as placeholder:

image

@oscfdezdz oscfdezdz marked this pull request as ready for review November 20, 2023 11:14
@oscfdezdz
Copy link
Collaborator Author

The main worry is that extension icons are very low-res. We'd probably want to avoid displaying pixellated or blurry icons, so it might be worth giving a maximum size (say 64x64) and always rendering at that resolution.

Setting pixel-size property to 64 should ensure that.

If the extension has one.
@mjakeman
Copy link
Owner

mjakeman commented Feb 5, 2024

Alright, I'm back! Time to start reviewing 😨

Thanks for this, looking good. We can tune the exact sizing/visuals later if needed

@mjakeman mjakeman merged commit 42c80db into master Feb 5, 2024
1 check passed
@mjakeman mjakeman deleted the oscfdezdz/extension-icon branch February 5, 2024 12:09
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.

None yet

2 participants