Closed GoogleCodeExporter closed 9 years ago
1) Style-wise, can you correct your patch to have
if (code like this) {
2) Did you make a change in the HTML to the "Bring to Front" and "Send to Back"
lines other than whitespace? Please revert if not.
3) var valid = !(typeof menuItem ==='undefined' || (menuItem == null) ...
can't this be replaced by
var valid = menuItem && ...
4) We haven't used console.error() in the code that I'm aware of. Is it
standard and supported everywhere?
5) In a future revision, it would be really great if the context menu stuff was
pulled out into a separate module (JS file) so that it's all in one place and
can be unit tested easier
6) Do you have a simple demo that shows this working?
Original comment by codedr...@gmail.com
on 23 Feb 2012 at 8:21
In order:
1) Style-wise, can you correct your patch to have...
I assume you are talking about white space here, and yes I can definitely get
proper formatting setup. Do you have a formatting guide or some other document
that describes your conventions?
2)Did you make a change in the HTML to the "Bring to Front" and "Send to Back"
lines other than whitespace? Please revert if not...
I can revert, was definitely just white space
3)var valid = !(typeof menuItem ==='undefined' || (menuItem == null) ...
I think you are correct, just getting overzealous with my insurance policy.
4) We haven't used console.error() in the code that I'm aware of. Is it
standard and supported everywhere?
I use error a lot in other JS apps, and especially in GWT apps so that it is
clear what kind of issues are being encountered. My background is much more
Java so I am used to being explicit about logging. As for standard support I
can vouch for webkit and firefox under firebug, beyond that Im not 100% sure. I
can find out for you if you'd like.
5) In a future revision, it would be really great if the context menu stuff
was pulled out into a separate module (JS file) so that ...
Absolutely, we are doing most of our work in separate extensions, and I am a
big believer in TDD. The reason I put this code here is that it seemed like
most of the initialization stuff and declaring of app wide resources was going
on in svg-editor.js. In this case though we aren't dependent on anything in
that scope so I will definitely move it out into a separate file.
6) Do you have a simple demo that shows this working?
I do, but I don't want to post the address in public so I will email you with a
url.
Finally,
This was the first of about 4 patches that I had for features I have been
working on. I will go back and correct them in accordance with your suggestions
and start submitting them.
Original comment by adamben...@gmail.com
on 23 Feb 2012 at 9:16
I have been trying to address the issue of making this context menu stuff a
separate module and I have a question regarding how to hook it into svg-edit. I
assumed that the correct way to do it was to make it an extension. If this is
correct I will have to add a line to the svg-editor.html to include the
extension. Is this the way you'd me to proceed?
Adam
Original comment by adamben...@gmail.com
on 23 Feb 2012 at 11:01
Attached are two patches which corrects the issues requested and also refactors
the contextmenu behavior into a separate extension.
Original comment by adamben...@gmail.com
on 23 Feb 2012 at 11:33
Attachments:
Hi Adam - the move to a separate JS file doesn't have to be done with this
change, but if you feel comfortable with it, great. Changes are along the
lines of what I did for refactoring in general:
* create a new JS file, like I did with browser.js or svgtransformlist.js, making a new, unique namespace in the svgedit namespace
* add it to the <!--{if svg_edit_release}> block in the svg-editor.html
* add it to the Makefile so that it gets compiled in teh release version
* add a JS unit test :)
Original comment by codedr...@gmail.com
on 24 Feb 2012 at 12:21
Oh, and I wont' have time to look at this until much later tonight or (more
likely) some time tomorrow evening :-/
Original comment by codedr...@gmail.com
on 24 Feb 2012 at 12:21
Got it, I will re-re factor out of extension and into module as described.
Thanks for your guidance.
Adam
Original comment by adamben...@gmail.com
on 24 Feb 2012 at 12:35
Oh, as for code style guide, we have none - but it can be picked up from
working with the code. Looking back, I think choosing tabs over a consistent
number of spaces was a bad idea so a whitespace-only refactor at some point in
the future would be cool if anyone is up for it.
Original comment by codedr...@gmail.com
on 24 Feb 2012 at 4:52
Looking at namespace options I can create a single svgEdit.contextMenu
namespace to collection all context menu relation functionality, or I can make
one just for this method svgEdit.addContextMenu... Do you have a preference?
Original comment by adamben...@gmail.com
on 24 Feb 2012 at 4:29
i would say svgedit.contextmenu sounds good to me. You don't have to move
everything in there initially. I'm all for iterating and not large-scale
refactors.
One nitpick is that all existing modules use 'svgedit' (all lower-case), so
please follow that convention:
var svgedit = svgedit || {};
(function() {
if (!svgedit.contextmenu) {
svgedit.contextmenu = {};
}
... your stuff here ...
})();
Original comment by codedr...@gmail.com
on 24 Feb 2012 at 5:24
Excellent, will have a patch for you shortly.
Original comment by adamben...@gmail.com
on 24 Feb 2012 at 5:31
Alright, I think I have what you are looking for. Attached is a patch that
factors context menu behavior into something like svgtransformlist.js. And
tests are included :)
Original comment by adamben...@gmail.com
on 24 Feb 2012 at 7:02
Attachments:
Thanks Adam. I've applied your patch in r2055. Once you've verified the
behavior, please close this bug.
One question: you say the dependencies are jQuery, but I just noticed there is
a jquery.contextMenu.js plugin in the source tree. Does your contextmenu.js
depend on that?
Original comment by codedr...@gmail.com
on 25 Feb 2012 at 2:15
Original comment by codedr...@gmail.com
on 25 Feb 2012 at 2:19
We depend directly on jQuery for the dom insertion of a new context menu, and
indirectly we expect contextMenu is around to trigger the rest of the context
menu behavior, however if it isn't around out changes wont fail to work you
just wouldnt see any context menus at all.
Original comment by adamben...@gmail.com
on 25 Feb 2012 at 9:33
Thanks for fighting through the muddled patch.
Original comment by adamben...@gmail.com
on 25 Feb 2012 at 9:45
Original issue reported on code.google.com by
adamben...@gmail.com
on 23 Feb 2012 at 6:04Attachments: