Complete web purchase from message (bug 1101995)#49
Conversation
There was a problem hiding this comment.
I think this will work the same as location e.g. you could also use just a.origin when available or fallback to the a.protocol + '//' + a.host as that should be enough to deal with the port.
There was a problem hiding this comment.
I thought that too but the unit test proved otherwise. It's weirdly inconsistent though.
There was a problem hiding this comment.
Looks ok to me from this http://jsbin.com/nogici/1/edit?html,js,console
There was a problem hiding this comment.
Oh, I didn't know origin was available there. Cool, thanks. Updated.
|
@muffinresearch after mozilla/spartacus#177 landed, I tested on dev and it works! ^5 Let me know when you've finished an r on it. |
There was a problem hiding this comment.
Extra space before ;
|
r+wc |
There was a problem hiding this comment.
I think this ship has probably sailed and this is personal preference, but I don't much like the exports.foo = function whatever style. It makes for long lines, plus when you want to refer to the func defined elsewhere you have to refer to it off of exports.
There was a problem hiding this comment.
My original reasoning for it was so one doesn't have to repeat the function name down below in the returned dictionary. However, I've noticed it's always helpful to use a named function so one has to repeat the name anyway. Thus, I could come around to always using named functions and exporting them at the bottom. Whichever style we go with, it just needs to be consistent throughout.
kumar303
force-pushed
the
pay-finish
branch
4 times, most recently
from
bbeca43 to
3a7493b
Compare
There was a problem hiding this comment.
maybe return a.origin || a.protocol + '//' + a.host; ?
Complete web purchase from message (bug 1101995)
2 participants
@muffinresearch r?
I've only tested with a dummy html page but it should work with your patch!