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

check response from parseCSSColor before downgrading it as it could be null #2006

Closed
wants to merge 1 commit into from
Closed

Conversation

andrewharvey
Copy link
Collaborator

No description provided.

@lucaswoj
Copy link
Contributor

This looks great! Thank you @andrewharvey.

I just have one small request before merging: parseColor was recently factored out of StyleDeclaration and currently all our color parsing tests live in style_declaration.test.js. Could you either move all the tests to parse_color.test.js or move your test to style_declaration.test.js?

@andrewharvey
Copy link
Collaborator Author

@lucaswoj Oh I didn't see those tests in style_declaration.test.js, I've updated the commit to include my test in with the others.

@lucaswoj
Copy link
Contributor

Looks great to me @andrewharvey! 🙇

I'm going to give the rest of the team a few more hours to chime in, then merge this into master.

colorCache[input] = output;
return output;
var parsedColor = parseCSSColor(input);
if (parsedColor !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a guard clause (if (!parsedColor) { throw new Error('Invalid color ' + input); }) and unindent the happy code path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the style advise. Agreed, much better that way. I've updated the commit.

@jfirebaugh
Copy link
Contributor

👍 Thank you!

@lucaswoj
Copy link
Contributor

🚢 in 6a18734

@lucaswoj lucaswoj closed this Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants