Skip to content

Clearer error message#349

Merged
k4b7 merged 4 commits into
Khan:masterfrom
galencorey:clearer-error-message
Apr 6, 2019
Merged

Clearer error message#349
k4b7 merged 4 commits into
Khan:masterfrom
galencorey:clearer-error-message

Conversation

@galencorey
Copy link
Copy Markdown
Contributor

closes #265

Add a more explicit error message when someone tries to pass a plain object instead of a stylesheet into the css() method.

Note: My editor went crazy with the double quotes. Let me know if you would like me to switch them back!

@khanbot
Copy link
Copy Markdown

khanbot commented Nov 12, 2018

Hey @galencorey,

Thanks for the PR! Mind signing our Contributor License Agreement?

When you've done so, go ahead and comment [clabot:check] and I'll check again.

Yours truly,
khanbot

@galencorey
Copy link
Copy Markdown
Contributor Author

[clabot:check]

@khanbot
Copy link
Copy Markdown

khanbot commented Nov 12, 2018

CLA signature looks good 👍

Copy link
Copy Markdown
Contributor

@k4b7 k4b7 left a comment

Choose a reason for hiding this comment

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

The change and the test looks good. The style changes to the test file are unrelated though. Please revert them.

Comment thread src/inject.js
};

const isValidStyleDefinition = (def /* : Object */) =>
"_definition" in def && "_name" in def && "_len" in def;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was hoping to make something like this public for #306.

Comment thread src/inject.js
definitionBits.push(styleDefinitions[i]._definition);
length += styleDefinitions[i]._len;
} else {
throw new Error("Invalid Style Definition: Styles should be defined using the StyleSheet.create method.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment thread tests/index_test.js

it("throws a useful error for invalid arguments", () => {
assert.throws(() => css({ color: "red" }), "Invalid Style Definition");
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice test.

@galencorey galencorey force-pushed the clearer-error-message branch from bceb252 to 7248751 Compare April 5, 2019 16:34
@galencorey
Copy link
Copy Markdown
Contributor Author

@kevinbarabash thank you for your review! reverted all the linter changes. Prettier has no chill sometimes.

Copy link
Copy Markdown
Contributor

@k4b7 k4b7 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for reverting the style changes.

@k4b7 k4b7 merged commit 1211bea into Khan:master Apr 6, 2019
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.

Using an object instead of StyleSheet.create causes css to throw difficult to trace error

3 participants