-
Notifications
You must be signed in to change notification settings - Fork 13
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
Refactoring #51
base: master
Are you sure you want to change the base?
Refactoring #51
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,167 @@ | ||
jQuery(function($) { | ||
var commands = { | ||
bold: function() { | ||
var weight = this.element.css("fontWeight"); | ||
|
||
if (weight === "bold" || weight > 500) { | ||
this.element.css("fontWeight", 400); | ||
} else { | ||
this.element.css("fontWeight", 700); | ||
} | ||
}, | ||
italic: function() { | ||
var style = this.element.css("fontStyle"); | ||
|
||
if (style === "italic") { | ||
this.element.css("fontStyle", "normal"); | ||
} else { | ||
this.element.css("fontStyle", "italic"); | ||
} | ||
}, | ||
underline: function() { | ||
var decoration = this.element.css("textDecoration"); | ||
|
||
if (decoration === "underline") { | ||
this.element.css("textDecoration", "none"); | ||
} else { | ||
this.element.css("textDecoration", "underline"); | ||
} | ||
}, | ||
fontSize: function(value) { | ||
if (value) { | ||
return this.element.css("fontSize", value); | ||
} else { | ||
return this.element.css("fontSize"); | ||
} | ||
}, | ||
fontFamily: function(value) { | ||
if (value) { | ||
return this.element.css("fontFamily", value); | ||
} else { | ||
return this.element.css("fontFamily"); | ||
} | ||
}, | ||
color: function(value) { | ||
if (value) { | ||
return this.element.css("color", value); | ||
} else { | ||
return this.element.css("color"); | ||
} | ||
}, | ||
backgroundColor: function(value) { | ||
if (value) { | ||
return this.element.css("backgroundColor", value); | ||
} else { | ||
return this.element.css("backgroundColor"); | ||
} | ||
}, | ||
textAlign: function(value) { | ||
if (value) { | ||
return this.element.css("textAlign", value); | ||
} else { | ||
return this.element.css("textAlign"); | ||
} | ||
}, | ||
unorderedList: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create lists on plain text editors"); | ||
} | ||
}, | ||
orderedList: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create lists on plain text editors"); | ||
} | ||
}, | ||
blockquote: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h1: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h2: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h3: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h4: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h5: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
h6: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
subscript: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
}, | ||
superscript: function() { | ||
if (this.__type === "plainText") { | ||
throw new Error("Can't create elements on plain text editor"); | ||
} | ||
} | ||
}; | ||
|
||
$.each([ | ||
"bold", "italic", "underline", | ||
"fontSize", "fontFamily", | ||
"color", "backgroundColor", | ||
"unorderedList", "orderedList", | ||
"textAlign" | ||
], function() { | ||
var command = this; | ||
Arte.prototype[command] = function(options) { | ||
return this.exec(command, options); | ||
}; | ||
}); | ||
|
||
$.each([ | ||
"blockquote", | ||
"h1", "h2", "h3", "h4", "h5", "h6", | ||
"subscript", "superscript", | ||
], function() { | ||
var command = this; | ||
Arte.prototype[command] = function() { | ||
return this.exec(command); | ||
}; | ||
}); | ||
|
||
$.extend(Arte.prototype, { | ||
exec: function(command, options) { | ||
var result; | ||
|
||
this.triggerEvent({ | ||
type: "arte-beforecommand", | ||
options: options, | ||
command: command | ||
}); | ||
|
||
result = commands[command].call(this, options); | ||
|
||
this.triggerEvent({ | ||
type: "arte-command", | ||
options: options, | ||
command: command | ||
}); | ||
|
||
return result; | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,156 @@ | ||
/** | ||
* @fileoverview Rich text editor | ||
* Usage: | ||
* 1) var arte = Arte(element); | ||
* Converts the first matched element into a rich text editor using default options and returns an existing instance | ||
* @returns {an Arte object} | ||
* 2) var arte = Arte(element, { options }); | ||
* Converts the first matched elements into a rich text editor using the options supplied and returns an existing instance | ||
* @returns {an Arte object} | ||
* | ||
*/ | ||
|
||
jQuery(function($) { | ||
function Arte(element, options) { | ||
if (!(this instanceof Arte)) { | ||
return new Arte(element, options); | ||
} | ||
|
||
this.container = $(element).first(); | ||
this.options = $.extend({ | ||
editorType: "plainText", | ||
styles: { | ||
"min-height": "200px", | ||
"height": "inherit" | ||
}, | ||
classes: [], | ||
value: "Please enter text ..." | ||
// TODO: set all default values here | ||
}, options); | ||
|
||
if (!this.container.length) { | ||
throw "Arte requires a DOM element"; | ||
} | ||
|
||
// NEW: replace element reference to the new made element | ||
// after it's initialized | ||
this.element = this.__initialize(); | ||
|
||
this.__initEvents(); | ||
|
||
this.__type = this.options.editorType; | ||
|
||
return this; | ||
} | ||
|
||
// Export Arte | ||
window.Arte = Arte; | ||
|
||
Arte.prototype = { | ||
__initialize: function() { | ||
var newElement; | ||
|
||
if (this.options.editorType === "richText") { | ||
newElement = $("<div/>", { | ||
class: "arte-richtext", | ||
contenteditable: true | ||
}) | ||
.html(this.options.value); | ||
} else { | ||
newElement = $("<textarea/>", { | ||
class: "arte-plaintext", | ||
style: { | ||
height: "100%", | ||
width: "100%", | ||
padding: 0, | ||
border: 0 | ||
} | ||
}) | ||
.val(this.options.value); | ||
} | ||
|
||
newElement | ||
// Apply default style | ||
.css(this.options.styles) | ||
// add custom classes | ||
.addClass(this.options.classes.join(" ")) | ||
// insert newElement to the element container | ||
.appendTo(this.container); | ||
|
||
return newElement; | ||
}, | ||
|
||
/* Methods */ | ||
|
||
/** | ||
* Get or Set content of the element | ||
* @params {string} value string to set content of element | ||
* @returns {string} returns 'innerHTML' of the contentEditable element | ||
* if in rich text mode or 'value' of the element if in plaintext mode | ||
*/ | ||
value: function() { | ||
if (this.__type === "plainText") { | ||
return $.fn.val.apply(this.element, arguments); | ||
} else { | ||
return $.fn.html.apply(this.element, arguments); | ||
} | ||
}, | ||
|
||
/** | ||
* Gets outerHtml of the element | ||
* @returns {string} returns 'outerHTML' of the element | ||
*/ | ||
outerValue: function() { | ||
return this.element.get(0).outerHTML; | ||
}, | ||
|
||
/** | ||
* Calls a focus event on the contentEditable element, moves the cursor | ||
* to the end of that element, and fires an onselectionchange event | ||
*/ | ||
focus: function() { | ||
var elem = this.element[0]; | ||
var range; | ||
var sel; | ||
|
||
this.element.trigger("focus"); | ||
|
||
// Moves the cursor to the end of the element | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want to have this as a utility method (like this) in the future. |
||
if (elem.selectionStart >= 0) { | ||
// Works only on textarea elements | ||
elem.selectionStart = elem.selectionEnd = elem.value.length; | ||
} else if (typeof elem.createTextRange !== "undefined") { | ||
// Fallback for textarea elements without support to selectionStart | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the browser compatibility (especially with regards to IE) for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a functionality research (to be sure it works) but forgot to document this anywhere. Even though, I'll run all of these tests on browsertack (with all IE flavors) in a near future so we can check that it's working just fine. Also, we can set checkpoints to see where it works or not. Our coverage implementation will surely tell one of these |
||
range = elem.createTextRange(); | ||
range.collapse(false); | ||
range.select(); | ||
} else if (this.__type === "richText" && document.createRange && window.getSelection) { | ||
// Works only on contenteditable | ||
range = document.createRange(); | ||
sel = window.getSelection(); | ||
|
||
while (elem.lastChild) { | ||
elem = elem.lastChild; | ||
} | ||
range.setStart(elem, elem.nodeValue.length); | ||
range.collapse(true); | ||
|
||
sel.removeAllRanges(); | ||
sel.addRange(range); | ||
} | ||
|
||
return this; | ||
}, | ||
|
||
destroy: function(options) { | ||
this.triggerEvent("arte-destroy"); | ||
|
||
if (options && options.removeContent) { | ||
this.element.empty(); | ||
} else { | ||
this.element.remove(); | ||
} | ||
} | ||
}; | ||
|
||
}); |
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.
these values (such as font-weight) will probably need to live in the configuration right? also it looks like commands need to support toggling classes on the elements as well - https://github.com/vistaprint/ArteJS/blob/master/src/editor/core/Commands.js#L198
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.
The configuration is creating a big object to live on the memory for no re-usability. There's no need so far to use it, as
fontWeight
won't be changed for anything else in any expected future.That's something we can add to the tests (even if it fails in the start) to add the feature later. This refactoring is still in progress and it is following the current documentation.
I'll write documentation for this class toggle method and ask you to review. Doing that we can ship all the necessary features.
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.
although the configuration is a big object there are many places around the code base that need to have access to these configurations - for example the state detector plugin - https://github.com/vistaprint/ArteJS/blob/master/src/editor/plugins/StateDetector.js#L68
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.
good, but this need to be progressive, step by step with docs, tests and code. If that is required by the state detector plugin, we add the feature.