Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for hex values, rgba strings, malformed values with fail… #419

Merged
merged 9 commits into from
Nov 8, 2018
Merged

Conversation

Lucassifoni
Copy link
Contributor

See issue #418 .
I added some test cases, but the coverage significantly dropped.
Still, function types match and should provide a safety net.
Have a nice day !

@Lucassifoni
Copy link
Contributor Author

It seems the travis integration consistently fails with Node < 6, maybe specifying a node minimum version would benefit the project too !

@codecov
Copy link

codecov bot commented Sep 11, 2018

Codecov Report

Merging #419 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #419   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     20    +1     
  Lines         701    742   +41     
=====================================
+ Hits          701    742   +41
Impacted Files Coverage Δ
lib/_lu-utilities.js 100% <100%> (ø)
lib/lost-utility.js 100% <100%> (ø) ⬆️

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 731a934...424b9b4. Read the comment docs.

@peterramsing
Copy link
Owner

@Lucassifoni This looks awesome. It looks like we need to get some tests in place to validate the fixes. Let me know if you need any help writing those tests. I'd love to help!

@peterramsing peterramsing self-requested a review September 11, 2018 02:59
Copy link
Owner

@peterramsing peterramsing left a comment

Choose a reason for hiding this comment

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

This looks like some pretty swell code! It needs some test coverage and with that, I'd say it's a sweet addition. Thank you!

return [0, 0, 255];
}

var testRgb = new RegExp(/rgb/);
Copy link
Owner

Choose a reason for hiding this comment

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

It sounds silly, but could this either be at the top or the bottom of the file? In the middle for a "const" feels a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated organization and added tests, it should be green for both tests & lints now.
Thanks for your guidance.

@peterramsing
Copy link
Owner

It also looks like a lint failed here: https://travis-ci.org/peterramsing/lost/jobs/427015147#L1112.

I've been looking into the lint config. I'm hesitant to turn this off but it has been falsely failing. Feel free to add //eslint-ignore-next-line if you feel it's needed.

@Lucassifoni
Copy link
Contributor Author

@peterramsing Hi Peter, thanks for your feedback.
I'll update this with tests and reorganize the code this morning !

@peterramsing
Copy link
Owner

This is lovely. I'm inspired by your code style. I'm gonna do some refactors of my own. Thanks for some awesome code!

@peterramsing
Copy link
Owner

...as for the package-lock? meh... I'm not sure why that's there but I'm doing some refactors that will blow that away so I'm not too worried about that.

@peterramsing peterramsing merged commit ec67f1d into peterramsing:master Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants