master: .elgg-icon smaller than in 1.x #8733

Closed
iionly opened this Issue Jul 13, 2015 · 14 comments

Comments

Projects
None yet
5 participants
@iionly
Contributor

iionly commented Jul 13, 2015

Problem is that the Fontawesome icons seem to be smaller (they look much too small for my liking!) or at least they are not aligned in the same way as the icons were aligned on 1.X.

For example, replacing the old elgg-icon-delete icon (used with <div class='elgg-icon elgg-icon-delete'>) with the new way of getting the icon to show (elgg_view_icon('delete')) the Fontawsome icon is slightly too low aligned in relation to the surrounding elements.

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Jul 13, 2015

Contributor

Oh great! Alignment within Elgg core is alright. It's just no longer right for 3rd party plugins. Now how to fix that? I'm beginning to tend towards using just my own set of icons within my plugins and no longer care about consistency - especially as I think that the fontawesome icons are just too small and using "fa-lg" doesn't seem to work either.

Contributor

iionly commented Jul 13, 2015

Oh great! Alignment within Elgg core is alright. It's just no longer right for 3rd party plugins. Now how to fix that? I'm beginning to tend towards using just my own set of icons within my plugins and no longer care about consistency - especially as I think that the fontawesome icons are just too small and using "fa-lg" doesn't seem to work either.

@PerJensen

This comment has been minimized.

Show comment
Hide comment
@PerJensen

PerJensen Jul 14, 2015

Member

they look much too small for my liking!

Default size can be changed in elements/icons.css

Member

PerJensen commented Jul 14, 2015

they look much too small for my liking!

Default size can be changed in elements/icons.css

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Jul 14, 2015

Contributor

I don't think so.

You can increase the width/height of the area reserved to icons in elements/icon.css but this won't result in the icons being larger as the icon images used will stay the same.

The font awesome icons will only work when using either elgg_view_icon(), e.g. elgg_view_icon('delete') or if using <div class='elgg-icon fa fa-times'></div> (meaning the old <div class='elgg-icon elgg-icon-delete'></div> won't work anymore at all).

The icons of the fa class seem to me to be slightly smaller than the old Elgg icons of Elgg 1.X (probably too small for small screens / touchscreens). There are other fa classes available (fa-lg, fa-2x etc.) that would display larger icons but using these classes doesn't work at all (tried it within a div directly and also by changing the fa class to be used in output/icons.php where the icon mapping is done). If you ask me, there should be fa-lg used at least instead of fa or it should at least work to change the class used.

Contributor

iionly commented Jul 14, 2015

I don't think so.

You can increase the width/height of the area reserved to icons in elements/icon.css but this won't result in the icons being larger as the icon images used will stay the same.

The font awesome icons will only work when using either elgg_view_icon(), e.g. elgg_view_icon('delete') or if using <div class='elgg-icon fa fa-times'></div> (meaning the old <div class='elgg-icon elgg-icon-delete'></div> won't work anymore at all).

The icons of the fa class seem to me to be slightly smaller than the old Elgg icons of Elgg 1.X (probably too small for small screens / touchscreens). There are other fa classes available (fa-lg, fa-2x etc.) that would display larger icons but using these classes doesn't work at all (tried it within a div directly and also by changing the fa class to be used in output/icons.php where the icon mapping is done). If you ask me, there should be fa-lg used at least instead of fa or it should at least work to change the class used.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 14, 2015

Member

Ideally we wouldn't set the font-size of elgg-icon at all and let it display inheriting from surrounding text. If you want to A/B these visually and choose a more precise font-size (currently 16px) or vertical-align value to match the old ones, we could do that.

Member

mrclay commented Jul 14, 2015

Ideally we wouldn't set the font-size of elgg-icon at all and let it display inheriting from surrounding text. If you want to A/B these visually and choose a more precise font-size (currently 16px) or vertical-align value to match the old ones, we could do that.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 14, 2015

Member

If it wasn't made clear, the FA "icons" are font glyphs, so font-size, vertical-align, color, etc. all can be used to adjust them.

Member

mrclay commented Jul 14, 2015

If it wasn't made clear, the FA "icons" are font glyphs, so font-size, vertical-align, color, etc. all can be used to adjust them.

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Jul 14, 2015

Contributor

I realized that I tried the "fa-lg" icons in the wrong way. It's not "fa-lg" instead of "fa" but rather "fa fa-lg" in output/icons.php.

@Steve: removal of font-size in icon.css.php sounds good (not to forget also to change it in Aalborg theme as I just had to learn...). Without font-size (or at least increasing it) it would also work with larger fontawesome icons ("fa fa-lg" instead of "fa" only).

Alignment issue is caused by css of the plugin I tried it with. This explains then also why the alignment for example in the entity menu of Elgg core was right but not with the custom icon added by this plugin.

Contributor

iionly commented Jul 14, 2015

I realized that I tried the "fa-lg" icons in the wrong way. It's not "fa-lg" instead of "fa" but rather "fa fa-lg" in output/icons.php.

@Steve: removal of font-size in icon.css.php sounds good (not to forget also to change it in Aalborg theme as I just had to learn...). Without font-size (or at least increasing it) it would also work with larger fontawesome icons ("fa fa-lg" instead of "fa" only).

Alignment issue is caused by css of the plugin I tried it with. This explains then also why the alignment for example in the entity menu of Elgg core was right but not with the custom icon added by this plugin.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 14, 2015

Member

To my eyes, font-size: 18px is closer to the size of the old icons overall. Vertical-align looks identical (I put a delete icon directly between two words). We should not be setting width or height I think.

Member

mrclay commented Jul 14, 2015

To my eyes, font-size: 18px is closer to the size of the old icons overall. Vertical-align looks identical (I put a delete icon directly between two words). We should not be setting width or height I think.

@mrclay mrclay changed the title from Fontawsome icons vertical alignment wrong to master: .elgg-icon font-size smaller than in 1.x. Jul 14, 2015

@mrclay mrclay changed the title from master: .elgg-icon font-size smaller than in 1.x. to master: .elgg-icon smaller than in 1.x Jul 14, 2015

@mrclay mrclay added this to the Elgg 2.0.x milestone Jul 14, 2015

@mrclay mrclay added the ui label Jul 14, 2015

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Jul 14, 2015

Contributor

Align issue is not an issue actually. I had some margin added within the plugin to get the alignment of the custom icon used with the 1.X icons, so I would have to change / remove this margin within the plugin for 2.X.

But the wrong alignment with this custom icon only became noticeable on 2.X because the fa icons are smaller (at least displayed smaller than the old icons).

With an increase to 18 - 20 px seems the fa icons seem to match the old icons in size. Even with 22 px they might not be too large, yet (thinking of think fingers on small touchscreens...). But then the tiny avatar might look too small beside an icon (e.g. in topbar).

Contributor

iionly commented Jul 14, 2015

Align issue is not an issue actually. I had some margin added within the plugin to get the alignment of the custom icon used with the 1.X icons, so I would have to change / remove this margin within the plugin for 2.X.

But the wrong alignment with this custom icon only became noticeable on 2.X because the fa icons are smaller (at least displayed smaller than the old icons).

With an increase to 18 - 20 px seems the fa icons seem to match the old icons in size. Even with 22 px they might not be too large, yet (thinking of think fingers on small touchscreens...). But then the tiny avatar might look too small beside an icon (e.g. in topbar).

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Aug 4, 2015

Member

I agree the icons should be larger by default.

Member

juho-jaakkola commented Aug 4, 2015

I agree the icons should be larger by default.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Aug 5, 2015

Member

@iionly want to submit a PR for this?

For whoever takes this on: would help to have before/after screenshots attached to the PR.

Member

ewinslow commented Aug 5, 2015

@iionly want to submit a PR for this?

For whoever takes this on: would help to have before/after screenshots attached to the PR.

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Aug 5, 2015

Contributor

I don't know if I will have any time to provide a PR in the near future.

Contributor

iionly commented Aug 5, 2015

I don't know if I will have any time to provide a PR in the near future.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Aug 5, 2015

Member

I'm currently working almost full time on a 2.0 project, so I will fix this this sooner or later. Cannot say when exactly.

Member

juho-jaakkola commented Aug 5, 2015

I'm currently working almost full time on a 2.0 project, so I will fix this this sooner or later. Cannot say when exactly.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Aug 14, 2015

Member

Additional problem is that the glyphs themselves have different sizes despite having the same font size.

For example fa-users uses much more of the available space than fa-remove:

fa-icons

This is caused partly by the fact that the remove icon isn't really a remove icon but an alias for "times" icon (as in "2 times 3 equals 6"). So in a way this is a design flaw in Font awesome.

We could define icon specific font sizes, but IMO that would be hacky.

I'll look into Font awesome issue tracker and see if anyone else has reported anything similar.

Member

juho-jaakkola commented Aug 14, 2015

Additional problem is that the glyphs themselves have different sizes despite having the same font size.

For example fa-users uses much more of the available space than fa-remove:

fa-icons

This is caused partly by the fact that the remove icon isn't really a remove icon but an alias for "times" icon (as in "2 times 3 equals 6"). So in a way this is a design flaw in Font awesome.

We could define icon specific font sizes, but IMO that would be hacky.

I'll look into Font awesome issue tracker and see if anyone else has reported anything similar.

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Aug 14, 2015

Member

Based on the issue tracker Font awesome has a lot of problems with consistency.

This issue FortAwesome/Font-Awesome#1540 might improve the size of the delete icon, but the style is quite different from what Elgg users are used to. (Also the issue is still unresolved.)

I guess at this point the only way to fix the inconsistencies is to define font-size separately for each icon.

Something like font-size: larger; might be a good approach because that way sites could still define the base size for all icons using a single CSS selector.

Member

juho-jaakkola commented Aug 14, 2015

Based on the issue tracker Font awesome has a lot of problems with consistency.

This issue FortAwesome/Font-Awesome#1540 might improve the size of the delete icon, but the style is quite different from what Elgg users are used to. (Also the issue is still unresolved.)

I guess at this point the only way to fix the inconsistencies is to define font-size separately for each icon.

Something like font-size: larger; might be a good approach because that way sites could still define the base size for all icons using a single CSS selector.

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Aug 14, 2015

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Aug 18, 2015

fix(icons): sizes of Font awesome icons are now more consistent with …
…old icons

 - Individually increases the size of some icons that were looking too small
 - Removes fixed width and height from icon containers
 - Increases default icon font size from 16px to 18px

Fixes #8733, #8861

juho-jaakkola added a commit to juho-jaakkola/Elgg that referenced this issue Aug 18, 2015

fix(icons): sizes of Font awesome icons are now more consistent with …
…old icons

 - Individually increases the size of some icons that were looking too small
 - Removes fixed width and height from icon containers
 - Increases default icon font size from 16px to 18px

Fixes #8733, #8861
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment