SnowdogApps / magento2-theme-blank-sass

SASS based version of Magento 2 Blank theme
MIT License
383 stars 150 forks source link

`$_icon-font-display` has no effect in the icon-font mixin #155

Closed mattgreenfield closed 7 years ago

mattgreenfield commented 7 years ago

This line; https://github.com/SnowdogApps/magento2-theme-blank-sass/blob/90da09911068b8c2c61d97e504014bb0ce8ab296/styles/vendor/magento-ui/_icons.scss#L314

... is hard coding icons to be display inline-block. Meaning that passing that the following mixin call doesn't update the icon to 'block'

 @include lib-icon-font(
        $_icon-font-display: block
    );

See - https://github.com/SnowdogApps/magento2-theme-blank-sass/blob/master/styles/vendor/magento-ui/_icons.scss#L21

I can make a PR to fix this, can you confirm that it's an issue and not a misunderstanding from myself please?

Thanks.

Leland commented 7 years ago

is hard coding icons to be display inline-block. Meaning that passing that the following mixin call doesn't update the icon to 'block'

This is incorrect – $_icon-font-display is setting the display of the element itself, and then the inline-block is setting the display of the pseudo element. This is behavior I would expect.

mattgreenfield commented 7 years ago

Sorry yes you are right. There is still no way to make the icon display block though. Say if I want an icon with text above or below it.

Leland commented 7 years ago

There should be. For instance:

<p>Above <i class="arrow"></i> Below</p>
.arrow {
  @include lib-icon-font(
        $icon-down,
        $_icon-font-display: block
  );
}
mattgreenfield commented 7 years ago

Yer, I'd like to be able to use this markup (editing markup up magento is such an effort!), <a href="/">Account</a> to display as ---[]--- Account

Yer, I can add another line of css to override the display inline-block the mixin is setting but it's more CSS.

Thanks for your help

Leland commented 7 years ago

Ah, yeah – not what the mixin was designed for it seems. Since this is a port of Magento's UI components, you should probably just bite the bullet and add the extra 1-2 lines of CSS.

If you really want that behavior you could also just override the mixin file in your theme and change it accordingly.

Januszpl commented 7 years ago

@mattgreenfield unfrotunetly here I agree with Leland. However you can post it on Magento2 github and if they will change it we will make changes here as well. I will close this issue now. Feel free to open if needed.