peterramsing / lost

LostGrid is a powerful grid system built in PostCSS that works with any preprocessor and even vanilla CSS.
http://lostgrid.org
MIT License
4.5k stars 160 forks source link

Optimisation of gutter logic #393

Closed steve-holland closed 7 years ago

steve-holland commented 7 years ago

What kind of change is this? (Bug Fix, Feature...) Optimisation.

What is the current behavior (You can also link to an issue)

371 - As per issue raised.

What is the new behavior this introduces (if any) Simplifies output of gutter logic.

Does this introduce any breaking changes? No.

Does the PR fulfill these requirements?

Other Comments Docs I'll look at updating at a later date. Just incase the output this generates needs changing first.

codecov[bot] commented 7 years ago

Codecov Report

Merging #393 into develop will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #393   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           19     19           
  Lines          701    708    +7     
======================================
+ Hits           701    708    +7
Impacted Files Coverage Δ
lib/_lg-logic.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7ff5216...52633c2. Read the comment docs.

steve-holland commented 7 years ago

I'd really appreciate you having a good look around this merge as it does have far-reaching changes, which makes me quite nervous!

As I essentially have to perform part of the calculation calc is doing, I wasn't sure about how accurate the rounding of the result needs to be. I've gone for 5 decimal places, but it doesn't have to be. We'd just need to update the associated tests.

As always, constructive criticism and nitpicking gratefully received :)

steve-holland commented 7 years ago

I think some sort of tailboarding process would be great idea.

I'm starting to think this change (or at least in its current form isn't right). It feels very much as if I'm re-inventing the wheel, as there are already tools out there that simplify the calc statement. So I am beginning to wonder if we should just use one of those either as a dependency in our code, or just not do the optimisation and leave other tools like cssnano to do the tidy up during the users build.

I think with the rounding we could make it configurable, but then if we start rounding up/down to whole numbers, I've a suspicion we're then going to get issues raised about the columns being 1px out or something like that.

peterramsing commented 7 years ago

🤔 I think I agree with not doing the optimization might be the best option. It feels like it could be error prone. 😢

There is a possibility that adding in optimizations that don't require rounding would work but I'm not sure how much of a win that would be.

@codebysubtract Do you think we drop this feature?

steve-holland commented 7 years ago

@peterramsing Yes, now the more I think about this, the more I think it could end up doing more harm than good.

The beauty of Lost is how simple it is. If we start bolting on more and more configuration option, you start moving away from that. Plus, I'm sure most folks already use other plugins like cssnanowhich help with optimisation so it feels like a duplication of effort anyway.

peterramsing commented 7 years ago

@codebysubtract Well, the code looked great. Thanks for the work!