Overlay #386
Overlay #386
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #386 +/- ##
===========================================
+ Coverage 90.21% 91.66% +1.44%
===========================================
Files 16 16
Lines 644 672 +28
===========================================
+ Hits 581 616 +35
+ Misses 63 56 -7
Continue to review full report at Codecov.
|
| color = utilityArray[4] || '#e6f6ff'; | ||
|
|
||
| if(!unitsMatch(maxWidth, gutter)) { | ||
| throw decl.error('lost-utility: Please use the same units for width and gutter.', { plugin: 'lost', word: 'lost-utility' }); |
There was a problem hiding this comment.
Not for here, but we should make this a method we can more easily access anywhere–that way we can ensure consistency.
| expect(function(){ | ||
| return processor.process(input).css; | ||
| }).to.throw(CssSyntaxError, message); | ||
| }; |
There was a problem hiding this comment.
@codebysubtract Are you using the .editorconfig file? It should add new lines to the end of this file. This should absolutely be automated/linted if we're gonna "enforce" it–but it hopefully would help with having whitespace diffs.
I think this would be my only "nit-pick" in this PR.
There was a problem hiding this comment.
Nitpicking absolutely fine 👍 I generally use PHPStorm as my IDE which does put in the newline at the end of files, but I'm just trying out VSCode for javascript work and unfortunately doesn't use .editorconfig out of the box (as I've just found out), so completely missed this!
I've updated the file in question now.
There was a problem hiding this comment.
@peterramsing not sure why but one of the travis builds failed to start for some reason. I wasn't sure how you restarted it and it doesn't appear to retry on its own. Any ideas?
|
Hehe. Great minds think alike on the eslint config. 😄 |
|
Restarting the tests (for some reason one failed to start). Then it should be good to merge. |
What kind of change is this? (Bug Fix, Feature...)
Feature.
What is the current behavior (You can also link to an issue)
Solves #349
What is the new behavior this introduces (if any)
Adds Grid Overlay to
lost-utility.Does this introduce any breaking changes?
No, it's new functionality.
Does the PR fulfill these requirements?
Other Comments
I wasn't sure how to approach error handling for this, so I took a look at the discussion in #180 and it seemed that throwing
node.errorwas the way forward, so that's what I've done. I've added a helper function to test throwing of errors, again based on what was in #180. If I've gone about it in totally the wrong way, then that's ok, just let me know how you want it to happen going forward and I can update the code.One other thing I wasn't sure of is your thoughts on comments within the code. It was a bit tricky keeping track of whats going on in this, so I broke it down into steps and put a comment. If you'd rather they weren't in there, again just let me know and I'll remove them.