edit: This question is about jQuery refactoring. Basically I have a big block of code but I want to see if other folks can think of a better way to refactor it. Since I'm new to jQuery I'm not sure if I'm on the right track or not with my design
original post: I'm working on a bookmarklet which adds HTML elements to the page. I'm building a sidebar which is a div with a ul inside of it. I'm trying to keep my styles separate from my code and to also write things so that they will be easy to manage if I need to make changes in the future. Is it possible to refactor this code to make it cleaner/more efficient?
myObj = {
$sidebar: {},
createSidebar: function () {
var self = this, $undo = {}, $redo = {}, $email = {}, $reset = {}
self.$sidebar = $("<div id='myObj-sidebar'><ul></ul></div>");
$undo = $('<a></a>', {
id: 'sidebar-undo',
className: 'sidebar-item',
href: '#',
text: 'Undo',
click: function (e) {
//self.doUndo();
e.preventDefault();
}
});
$redo = $('<a></a>', {
id: 'sidebar-redo',
className: 'sidebar-item',
href: '#',
text: 'Redo',
click: function (e) {
//self.doRedo();
e.preventDefault();
}
});
$email = $('<a></a>', {
id: 'sidebar-change-email',
className: 'sidebar-item',
href: '#',
text: 'Change E-Mail',
click: function (e) {
//self.createEmailDialog();
e.preventDefault();
}
});
$reset = $('<a></a>', {
id: 'sidebar-reset-all',
className: 'sidebar-item',
href: '#',
text: 'Reset All',
click: function (e) {
//self.doReset();
e.preventDefau开发者_运维知识库lt();
}
});
self.$sidebar.find('ul').append($undo, $redo, $email, $reset);
self.$sidebar.find('.sidebar-item').wrap('<li/>');
self.$sidebar.appendTo("body");
}
}
I'm not really sure what the question is - my interpretation is that you're asking for refactoring advice. If that's what you're looking for, here's the way I'd probably implement the same requirements:
jQuery('<div><ul></ul></div>')
.appendTo("body")
.css({
background: "white",
left: 0,
position: "absolute",
textAlign: "left",
top: '50%'
})
.find('ul')
.css({
listStyleType: 'none',
margin: 0,
padding: 0
})
.append('<li><a href="" class="sidebar-undo">Undo</a></li>')
.find('.sidebar-undo')
.click(function(e){
e.preventDefault();
// your 'undo' code
})
.end()
.append('<li><a href="" class="sidebar-redo">Redo</a></li>')
.find('.sidebar-redo')
.click(function(e){
e.preventDefault();
// your 'redo' code
})
.end()
.append('<li><a href="" class="sidebar-changeEmail">Change E-Mail</a></li>')
.find('.sidebar-changeEmail')
.click(function(e){
e.preventDefault();
// your 'changeEmail' code
})
.end()
.append('<li><a href="" class="sidebar-doReset">Reset All</a></li>')
.find('.sidebar-doReset')
.click(function(e){
e.preventDefault();
// your doReset code
})
.end()
.find('li')
.css({
padding: '0.5em'
})
.end()
.find('a')
.css({
background: "whatever",
color: "etc"
});
Notes:
e.preventDefault()
should generally be the first line of your handler, that way if an exception is thrown later, the anchor will still not navigate (easier to debug);- you don't need to give any of your elements IDs, since you have direct DOM handles on everything already;
- your hrefs should ideally be useful, taking the user to a legitimate page if they right-click and do "open in new tab" (using "preventDefault" will keep them from navigating away on regular left-click);
- since it's a bookmarklet, don't worry too much about separating out your styles - you're using jQuery, so you can easily grab the elements you care about and apply CSS properties directly;
- the example above has no variables aside from the
e
event object - to me, this is a more elegant solution than creating aself
reference and a bunch of collection objects (that's just my opinion).
Hope this helps!
精彩评论