dmstr/yii2-adminlte-asset

FontAwesome 4.6.3

zertex opened this issue · 9 comments

zertex commented

Why used "rmrevin/yii2-fontawesome": "~2.9" with Font Awesome 4.6.3?
rmrevin/yii2-fontawesome 2.17 used 4.7.0

schmunk42 commented

We had some issues in the past with minor version upgrades in rmrevin/yii2-fontawesome AFAIR. So we went for patch level updates only.

Can you confirm things are working correctly with 2.17?

zertex commented

I don't know... I manually install
"fortawesome/font-awesome": "~4.7", "rmrevin/yii2-fontawesome": "~2.17"
now all ok

sashsvamir commented

adminLte already contains "font-awesome" bower package at vendor/almasaeed2010/adminlte/bower_components/font-awesome/, so why don't create FontawesomeAsset here and using adminlte assets instead require rmrevin/yii2-fontawesome with dependancy of "fortawesome/font-awesome"?
This way can reduce vendor directory size up to 13MB.

schmunk42 commented

We would need to create an additional asset-bundle, while there is already a rather widely used one (fortawesome). And where would we stop regarding all the other bower_components...

But nonetheless I am thinking about removing the composer-dependency along with the depends in the asset bundle, so a developer could choose the way he wants to install Fontawesome.

This would be at least a minor version update. Any thoughts?

CC: @githubjeka

  • 👍1
githubjeka commented

My opinion hasn't changed about this package

See a426168#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780

I use a personal bundle configured specifically for end project.

CyberPunkCodes commented

Remove any 3rd party FontAwesome dependancies. Your creating a daisy chain too long. Just use the CDN, which would eliminate composer errors (regarding this issue). Then they can simply change the version used in the head section of the HTML in their templates. 1 less script to host, 1 less script to combine/minify, and using the CDN means it's probably cached for the end user already since it's an extremely common library across the entire internet.

Your AdminLTE theme is no longer working properly with the latest (v5) of Font Awesome. A lot of the icon class names changed, fa is now deprecated, and some paddings/spacings are messed up. Not terrible, but some icons don't lay the same way.

You would need to refactor all of the places that FontAwesome is used and release a new major build version of your repo.

Let's take the 2 most obvious places where you use FA.

Left Sidebar:

When you replace the icons with their new class names (ie: fas fa-share) for each sidebar item, the text is right up against the icon, instead of spaced out 20px. This is because the binding is on the fa class name, which is now deprecated according to FA v5+.

I had to create a custom.css stylesheet to apply the fix to load after your adminlte css. Here was my fix for just the sidebar icons:

.sidebar-menu > li > a > .fa,
.sidebar-menu > li > a > .fab,
.sidebar-menu > li > a > .fal,
.sidebar-menu > li > a > .far,
.sidebar-menu > li > a > .fas,
.sidebar-menu > li > a > .glyphicon,
.sidebar-menu > li > a > .ion {
  width: 20px;
}

.treeview-menu > li > a > .fa,
.treeview-menu > li > a > .fab,
.treeview-menu > li > a > .fal,
.treeview-menu > li > a > .far,
.treeview-menu > li > a > .fas,
.treeview-menu > li > a > .glyphicon,
.treeview-menu > li > a > .ion {
  width: 20px;
}

I left the fa for backwards compatibility. If you don't know where in your dist\css\AdminLTE.css file these 2 sections correspond to, let me know and I can get you the line numbers. All I did was add these classes: fab, fal, far, fas. This should allow for FontAwesome 5 free and pro (paid) versions.

Header Nav:

Same thing, some of the icons don't work with FontAwesome 5. On the toggle, you inject the bars icon using before. This should probably just be in the HTML as <i class="fas fa-bars"></i>. To fix it the way you have it, you have to now specify a font-weight: 900; and the new font family font-family: "Font Awesome 5 Free";.

Here is my fix for the way you have it:

.main-header .sidebar-toggle {
  float: left;
  background-color: transparent;
  background-image: none;
  padding: 15px 15px;
  font-weight: 900;
  font-family: "Font Awesome 5 Free";
}

The icons on the right didn't work, so replace accordingly with: far fa-envelope, far fa-bell, far fa-flag. The width/padding seemed fine here. In your demo content, the exclamation icon (warning) was missing from the notifications dropdown. Replace with: fas fa-exclamation-triangle. I am not sure about the cog, because I removed it from this template.

Then of course, everywhere else that you rely on FontAwesome lol.

Fix

I think you should take everywhere in your CSS that targets the fa class, and also target the fab, fal, far, fas classes, like I did in the first fix up above. This would get your theme compatible with both v4 and v5, and rollout an update. I would stick with the latest v4 due to bc. They would have to edit the HTML to reference the new icons, but at least the paddings and spacings would line up out of the box.

Then revamp the whole repo for FontAwesome v5 as a new major version to not break bc. I don't think you should include dependancies, and just use the CDN. If they don't like it, they can use their own package of their choice, and wire up a build script via Grunt/Gulp. This would make your end easier, and easier for people to use the Free or Pro versions.

  • 👍1
schmunk42 commented

@WadeShuler Thank you for the insights.

But a general question, does AdminLTE support FontAwesome 5? I think they still stick with v4, right?

CyberPunkCodes commented

@schmunk42 They still use v4. Does that mean everyone has to still stick with the outdated version? I don't think that is a good reason to hinder every developer who wants to move forward. Who knows when they will roll out a new version. Over the next few months, I predict you will have more people creating issues regarding FA5 support.

At the very least, you should make $iconClassPrefix a configurable option on the Menu Class by removing the static scope. I copied the Menu class, and did just that (as well as fixing the reference to it in the code), and it works perfect. It allows us to remove the default prefix fa fa- when using icon on the menu, so we can use FA5 without breaking out of your package with our own modified Menu class.

schmunk42 commented

We'll use a composer dependency in the 2.x branch and remove it in 3.x