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

Set status icons to at least 16px #888

Conversation

jntesteves
Copy link
Contributor

No reason to be dynamic when the clock and other text are not. 16px always looks good and is readable. It was getting too small at panel-size 32, where it was 16px before ae2b52e and bb6742a.

@HalfVoxel
Copy link
Contributor

I dunno. It looks kinda weird with a wide panel and tiny icons.

@jntesteves
Copy link
Contributor Author

I dunno. It looks kinda weird with a wide panel and tiny icons.

How wide are we talking? Is there a use-case to go above 48? I don't have a HiDPI monitor to test,I though it was like in the browsers where the px unit is relative.

This PR doesn't change previous behavior at panel-size 48.

@jntesteves jntesteves force-pushed the feature/restore-status-icons-sizes branch from 46ca775 to ccc23b0 Compare September 30, 2022 01:58
@jntesteves jntesteves changed the title Set status icons to fixed 16px Set status icons to at least 16px Sep 30, 2022
@jntesteves
Copy link
Contributor Author

I changed it to be at least 16px, but scale larger on panel sizes above 48. I'm still not convinced this is necessary, but it doesn't hurt anyway. I only want to fix the icons being currently too small at PS 32.

@PapyElGringo
Copy link
Collaborator

@HalfVoxel you have the final word on this one 😁

Copy link
Contributor

@HalfVoxel HalfVoxel left a comment

Choose a reason for hiding this comment

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

Using maximum seems reasonable. I guess we will have problems if the panel size gets smaller than 16px... but that seems unlikely. And will probably break other things anyway.

@PapyElGringo PapyElGringo merged commit d269fc9 into material-shell:main Oct 5, 2022
@PapyElGringo
Copy link
Collaborator

Perfect thanks you guys !

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

3 participants