Search code examples
laravellaravel-5conventionsadminlte

AdminLTE themes: small disagreement with coworker (soft skills)

I have a quick question regarding implementation of a small change in our system, and I want to hear your opinion about my little disagreement with another developer in our company.

Our working environment:

  • Laravel
  • AdminLTE
  • Two laravel guards for 'partner' and 'staff'. Each type of user (partner/staff) has access to a different set of pages, using a different set of controllers and a different subdomain.

Admin LTE comes with some skins that you can apply to your <body>, for example 'skin-blue' theme. This is what our page looks like. Just for a comparison, if you remove the 'skin-blue' class, our website looks like this.

We were asked by our client to change the color of the top navbar for the Staff side. So, because the colors at the moment are being added by an adminLTE skin, I thought it was better to create a second theme for the staff side, calling it "skin-staff", and then in our base blade file, check for which guard is being used, and add the class accordingly.

<body class="@if(get_guard() === 'partner') skin-blue @else skin-staff @endif" ...>

I made a copy of the original skin-blue file, renamed it to skin-staff, and just changed the color of the necessary elements. I thought this was the best way to go about it, but the developer which had to review my github Pull Request said that because this was such a small change, it wasn't necessary to create a new skin. His proposed solution was to simply add the css classes in the blade file, something like: 

<head>

…

<style type="text/css">
@if (get_guard() === 'staff')
   .skin-blue .main-header .navbar{
       background-color:#bdac3c
   } 
   .skin-blue .main-header .navbar .sidebar-toggle:hover{
        background-color:#ac9b2b
   }
   .skin-blue .main-header .logo{
         background-color:#bdac3c;
   }

   … // and other classes
@endif
</style>

Now, to me this is not correct, because we are mixing the logic for staff and partner side without a clear way to differentiate them. If we use skins, we can simply say something like "The top navbar is yellow because we are using class skin-staff". And "We are using class skin-staff because we are on the Staff guard". The propositions are clear and simple. However, by adding raw CSS to our blade file, we end up with something like "The top navbar is yellow because we are using skin-blue and also we are on the Staff guard and also we have added some custom CSS for the Staff guard". The extra changes we introduce to the system don't follow the pattern used by adminLTE, to me they just look like noise. If we had to for example do this five more times, we would end up with a lot of CSS in our base blade file, which I think would look bad and would force us to eventually decide to use the skin system of adminLTE, something we could just do right away.

But, being as stubborn as I know I am, I don't know if I have the right idea or if I just want to do things my way.

What do you guys think? Is it better to create a new skin, even if most of the CSS code inside the skin file will be duplicated, but it allows us to stick to the existing way of doing things, or is it better to just add the code in the blade file and don't think more about it?

Thanks for your ideas

Solution

  • This is very much an opinion based question, there is no clear right or wrong answer here.

    Personally, I agree with your coworker, why copy the whole theme, that is hundreds of lines long, just to change a handful of classes?

    That said, I don't personally like the styles living in the DOM under a style tag.

    Why not create a new CSS file that contains the styles:

    .skin-blue .main-header .navbar{
    background-color:#bdac3c
} 
.skin-blue .main-header .navbar .sidebar-toggle:hover{
    background-color:#ac9b2b
}
.skin-blue .main-header .logo{
    background-color:#bdac3c;
}

… // and other classes

    And then as long as you include this file after the base skin-blue CSS theme, your updated staff skin changes will take precedence.

    Something like this:

    <link rel="stylesheet" href="{{ asset('css/skin-blue.css') }}">
@if (get_guard() === 'staff')
<link rel="stylesheet" href="{{ asset('css/skin-staff.css') }}">
@endif

    This keeps the abstraction of your CSS inside CSS files (and out of the DOM), yet only overwrites exactly what it needs to.

    It also means that if you need to update a common style between the two themes, you don't need to make the change in two different files; you just need to modify the skin-blue.css file.