Skip to content

Commit

Permalink
Merge pull request #8 from tjFogarty/master
Browse files Browse the repository at this point in the history 
Updated to produce less CSS when compiled
  • Loading branch information
@mastastealth
mastastealth committed Jan 10, 2014
2 parents 8586bcc + 084957b commit b3a8cb1
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
4 changes: 2 additions & 2 deletions flex.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
//
// http://w3.org/tr/css3-flexbox/#flex-containers

@mixin flexbox {
%flexbox {
display: -webkit-box;
display: -webkit-flex;
display: -moz-flex;
Expand All @@ -68,7 +68,7 @@

//----------------------------------

@mixin inline-flex {
%inline-flex {
display: -webkit-inline-box;
display: -webkit-inline-flex;
display: -moz-inline-flex;
Expand Down
4 changes: 2 additions & 2 deletions tests/tests.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

/*--------------------------------------------------------------------*/

.flexbox { @include flexbox; }
.flexbox-inline { @include inline-flex; }
.flexbox { @extend %flexbox; }
.flexbox-inline { @extend %inline-flex; }

/*--------------------------------------------------------------------*/

Expand Down

10 comments on commit b3a8cb1

@kpeatt
Copy link
Contributor

@kpeatt kpeatt commented on b3a8cb1 Jan 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change essentially breaks the mixin going forward. I'm now required to either extend a flexbox class whenever I want to add flex (creating crazy selector chains) or manually write my own mixin. This is a trivial thing for someone to use the mixin for if they desire — I don't see why it should be part of the mixin itself.

@mastastealth
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpeatt Why do you think so? I understand that if someone were to "update" the mixin to this, they would then need to go back and replace the includes to be extends, but then again, this is not the type of mixin you really need to update (all the prefixed/standard syntaxes are in place), I think. However, for future users, it would clean up their CSS a bit. You don't think the trade off is worth it?

@kpeatt
Copy link
Contributor

@kpeatt kpeatt commented on b3a8cb1 Jan 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mastastealth:

My main problem with this change is that it breaks the idea of the 'mixin'. Mixins are output-less utilities that people can include. As the mixin now stands, I have two decoration classes that I don't want and will now have to manually change every time the mixin updates. I will also have to update any past projects if bugfixes come up and I don't want to do manual diffs.

this is not the type of mixin you really need to update

Just looking through the past pull request history tells you that this mixin is something you need to update every so often.

However, for future users, it would clean up their CSS a bit

As for future users, I don't think this will clean up CSS at all. Gzip should take care of much of our repetition in terms of filesize. Instead, I think this will fracture the CSS between using a decoration class to declare your main flexbox intentions and then still requiring people to @include whenever they need to add another property.

My suggestion would be, if you're set on the extend, include the silent placeholder extends alongside the mixin. Then you can provide documentation on how to use the @extend without creating any default output.

@mixin flexbox {
+
}

%flexbox {
    @include flexbox;
}

@mastastealth
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid points. I'm aware gzip would definitely help the repetition bits, though I also liked the idea of the @extend basically gathering all the selectors that apply the display: flex into one rule. I'm not sure if you are referring to the change in the tests.css (using .flexbox with the extend) as the decoration class? The point wouldn't be to use that at all, rather, replace @include with @extend in the selectors that are defined as flexbox.

It's certainly not set in stone or anything. @tjFogarty, do you have any counterpoints that really drive the need for the include/extend swap? If not, I'll probably revert it to avoid the trouble @kpeatt mentioned.

@jeffkamo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, the impact of switching between the @mixin version and the @extend version, I found to be rather minimal after gzip.

I tested a project whose non-compressed stylesheet was 89kb using @mixin. Switching to @extend shrunk that down to 86kb.

However, after gzip, both versions were 14kb.

Mind you, this is a project with about 40+ instances of the @include/extend.

@jeffkamo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, if %placeholder classes stay then I agree that having the @mixin remain would be preferable.

That helps keep backwards compatibility and allows users to choose which method they prefer.

@tjFogarty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main point of concern was in reducing the amount of repetition seeing as it was a mixin that wasn't receiving any parameters. I was coming from the point of view of DRY (Don't Repeat Yourself) CSS. It just made sense to declare the rules once.

Also, for me at least, gzip isn't something that's always available. A lot of clients I work with are usually on shared hosting which has gzip disabled due to CPU usage potentially slowing down the response of the server. Or at least that's what I'm told by the main hosting company they use.

All in all, I'm alright with it going either way. It's only a few seconds worth of a change for users getting started with this.

@mastastealth
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, so "best of both worlds" seems to be the way to go here. Added 8f6698e per @kpeatt's suggestion. :) Thanks for the feedback guys.

@tjFogarty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, cheers :)

@kpeatt
Copy link
Contributor

@kpeatt kpeatt commented on b3a8cb1 Jan 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys!

Please sign in to comment.