-
Notifications
You must be signed in to change notification settings - Fork 69
Add support for custom preferences page #543
Conversation
review?(@kumar303) |
Thanks for the patch. This looks pretty straightforward. It also needs a unit test. You can run the suite with |
@kumar303 Thanks for taking a look! Added automated tests. |
expect(optType.firstChild.data).to.be.equal("3"); | ||
}); | ||
|
||
it("sets em:optionsType to 3 both `preferences` and `preferencesURL` exist", function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo: missing "if" in the sentence
The tests look great, thanks. Can you squash it into one commit? |
@kumar303 Done, thanks! |
Hmm. I'm not sure why the suite is failing now. Do you see these same failures locally? |
@kumar303 I can't reproduce locally with |
I retriggered the build once already, didn't seem to make a difference. Did you rebase on top of the latest master? Might be worth a try. |
The failures here seem to be the same as in #546 |
You can rebase on master to get a fix for the failing Firefox tests |
The commit message should also be prefixed with |
moved to #549 |
No description provided.