-
Notifications
You must be signed in to change notification settings - Fork 31
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
changed optional argument syntax of \img command (img.js extension) to be more LaTeX-like #14
Conversation
Packed source was not yet updated, as I didn't come around yet to actually install MathJax's toolchain. |
Thanks. I haven't had time to look in detail. But you can pack it any way you like; we don't have many for third-party extensions. Coudl you fill out http://www.mathjax.org/cla/ (and reply to the confirmation email)? We don't require this for third party extensions in general, but since this is one the MathJax team created and since it might eventually make it to the core repository, we'd need a CLA. Thanks! |
Sorry for the delay. This looks good in general. Perhaps the error messages could end in |
bump |
32c4014
to
9e7a2ac
Compare
like so? |
var opts = optarg.split(','); | ||
for(var i=0,l=opts.length;i<l;++i){ | ||
var parts = opts[i].split('='); | ||
var key = parts[0].trim(); |
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.
Note that the string trim()
function is not available in IE8 and below, so this will cause those browsers to die. Perhaps using parts[0].replace(/^ +/,"").replace(/ +$/,"")
would be better (if less efficient) here.
I clearly lost track of this. Can we merge this? |
Well, I had made some comments on the code that haven't been addressed. But since it was your extension to begin with, it is really up to you whether you want to accept the changes or not (or accept and then modify afterward). I'm happy in principle with it, but do thing those few things could be altered. |
Thanks, Davide. @burnpanck are you ok with merging it or do you want to modify this further? |
Sorry, my fault. I'm rather busy these days unfortunately. I did now apply the changes @dpvc suggested. With that I am ok to merge. |
Thanks, @burnpanck -- that's great! |
changed optional argument syntax of \img command (img.js extension) to be more LaTeX-like
Optional arguments are now given either as
or
which are both common ways to supply optional parameters in LaTeX (the latter is usually used when there is just one optional argument). Example in http://jsfiddle.net/c6sy534g/