-
Notifications
You must be signed in to change notification settings - Fork 85
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
Material design icon size #2248
Comments
Apparently, no one really cares to discuss this properly and we just go with a size of 20 px implicitly decided in nextcloud/documentation#7131. I am looking forward to the PRs adjusting this in order to bring some consistency (since we currently use 24 px everywhere). |
@raimund-schluessler in the example of nextcloud/calendar#3447 (review) the icons look a bit big tbh. We checked the different sizes in the button pull request, and 20 seemed very nice.
If we do that, we should do that uniformly though, not as a "by the way" of switching to Material Design Icons. :) A pretty important benchmark is that the icons have proper size compared to the text – which they do at the moment, and which also works in the work-in-progress button component. |
Oh and @raimund-schluessler of course we can keep the discussion open. :) We should just start with a value first to not have it block anything.
So 24 is the default when importing a Material Design icon, as per the library. But this default looks visually too large for our font size and considering we use filled icons, so this is why we checked what works well generally, and specifically with the button component, and size 20 seems good visually. Of course for specific things like the viewer (or video player or whatnot), icon size can be bigger. But we should make sure we use 1 specific size for our base size, kind of like we use 15px for the base body text. :) Yes it seems a bit of a pain that every app dev has to do this manually (as far as I understand), I’d also prefer if this would be baked in so that if font sizes change, we can also scale icon sizes centrally. |
I also like 20px better. Some icons will fill the whole width or height and some will be smaller. MD icons are designed to match visual balance between them rather than to match the dimensions to the pixel. |
So instead of having e.g. a slot in the button component for the icon sizes, we could check if it’s possible to just set a global icon size, then we have conformity by default, and we can play around which size works best overall (also in case we ever increase font size again)? :) |
Alright, thanks for the statements. And sorry, that I was a bit frustrated here, don't take it personally, please. I still think that 20 px will be to small (some icons will be even smaller than they are now with 16 px). However, at least we have a decision now. I will adjust the pending PRs and start changing the existing icon size. |
Closed with #2263. |
This issue is to discuss and decide the material design icon size. Since this will have quite a big impact on Nextcloud globally and will cause some effort adjusting already existing markup, I think we should come to a well thought through decision.
Currently, we use an icon size of 16 px for all Nextcloud icons. When switching to material design icons this roughly corresponds to
size="20"
on the vue-material-design-icons component (although it depends on the exact item used). On nextcloud-vue and the Tasks app (checkout c.nc.c) we currently usesize="24"
(also see #2247 and the netlify docs https://deploy-preview-2247--nextcloud-vue-components.netlify.app/#/Components/App%20containers/AppNavigation?id=appnavigationitem to play around and nextcloud/calendar#3447 for 24 px).This is how it looks in comparison:
I, personally, would like to increase the icon size a bit, and use size 22. With size 20 some icons are smaller than they currently are, and 16 px is quite small already, I think. E.g mail.google.com uses icons with a size of 20 px and this looks quite good, imho.
Please have a look @nextcloud/designers @nextcloud/vuejs.
The text was updated successfully, but these errors were encountered: