Image Filter client

Question

I've made an image filtering client for JavaScript that runs on jQuery, Vex and Fabric.JS.

You simply include the required files in your html page, along with something like:

<script type="text/javascript">
 ImageFilter.defaultOptions.imageCallback = server.upload();
</script>

and then have a <a><img /></a> structure where you add an empty data attribute (data-imagefilter) and a filled data attribute (data-imagefilter-image, which is filled with the src).

Once you click on the image, it opens up a popup and lets you edit and save, or edit and cancel a bunch of filters. This has issues running on the local machines are that cross-origin domains don't work, however, this is intended for use on a live environment.

 
/* ImageFilter.JS
 *
 * Dependencies:
 * - Browser Environment
 * - Fabric.js - fabricjs.com
 * - jQuery
 * - Vex - github.hubspot.com/vex
 */
 
(function(global) {
 "use strict";
 
 global.ImageFilter = {
 defaultOptions: {
 imageCallback: function(){}
 }
 };
 /* polyfills */
 var extend = jQuery.extend;
 
 function $(selector, el) {
 if (typeof selector !== "string") {
 return selector;
 };
 if (!el) { el = document; }
 return el.querySelector(selector);
 }
 
 function selectMultiple(selector, el) {
 if (!el) { el = document; }
 return Array.prototype.slice.call(el.querySelectorAll(selector));
 }
 /* end of polyfills */
 var ImageFilter = function() {
 if (!global) {
 throw new Error("window not found.");
 }
 if (!fabric) {
 throw new Error("Fabric.JS not found.");
 }
 if (!jQuery) {
 throw new Error("jQuery not found.");
 }
 if (!vex) {
 throw new Error("Vex not found");
 }
 this.defaultOptions = {};
 this.CANVAS_ID = "#_IFCanvas";
 this.filters = {
 'sepia': new fabric.Image.filters.Sepia(),
 'brightness100': new fabric.Image.filters.Brightness({ brightness: 100 }),
 'grayscale': new fabric.Image.filters.Grayscale()
 };
 selectMultiple("[data-imagefilter]", document).forEach(function($el) {
 $el.addEventListener("click", function() {
 global.ImageFilter.loadImage(this.dataset["imagefilterImage"]);
 });
 });
 };
 ImageFilter.version = "1.0";
 ImageFilter.prototype.loadImage = function(imageSrc) {
 var image = new Image;
 image.src = imageSrc;
 var canvas = document.createElement("canvas");
 canvas.id = this.CANVAS_ID.replace("#", "");
 canvas.width = image.width + 30;
 canvas.style.left = "50%";
 canvas.height = image.height + 20;
 var container = document.createElement('div');
 var ul = document.createElement('ul');
 ul.classList.add('theme-style-none');
 Object.keys(this.filters).forEach(function(filter){
 var li = document.createElement('li');
 var a = document.createElement('a');
 a.href = "#";
 a.text = filter;
 a.setAttribute('data-imageFilter-filter', filter);
 li.appendChild(a);
 ul.appendChild(li);
 })
 
 container.appendChild(canvas);
 container.appendChild(ul);
 
 var CANVAS_ID = this.CANVAS_ID;
 if (this.defaultOptions.imageCallback){
 var callback = this.defaultOptions.imageCallback;
 }
 vex.dialog.open({
 message: "",
 buttons: [
 extend({}, vex.dialog.buttons.YES, {
 text: "Done"
 }), extend({}, vex.dialog.buttons.NO, {
 text: "Back"
 })
 ],
 contentCSS: {
 "width": canvas.width + 20 + "px"
 },
 input: container
 }).bind("vexClose", function() {
 var imageData = $(CANVAS_ID).fabric.toDataURL('png');
 callback(imageData, imageSrc);
 });
 
 self.canvas = new fabric.Canvas(CANVAS_ID.replace("#", ""));
 $(CANVAS_ID).fabric = self.canvas;
 
 self.incomingImage = image;
 var imgInstance = new fabric.Image(image);
 imgInstance.selectable = false;
 self.canvas.selection = false; // disable group selection
 self.canvas.add(imgInstance);
 this.canvas = self.canvas;
 
 $$("[data-imageFilter-filter]").forEach(function($el){
 $el.addEventListener("click", function() {
 global.ImageFilter.applyFilter(this.dataset["imagefilterFilter"]);
 });
 });
 };
 ImageFilter.prototype.applyFilter = function(filter){
 console.log(arguments);
 var currentObject = this.canvas.getObjects()[0];
 currentObject.filters = [];
 currentObject.filters.push(this.filters[filter]);
 currentObject.applyFilters(
 this.canvas.renderAll.bind()
 );
 };
 window.onload = function() {
 global.ImageFilter = new ImageFilter();
 };
 
})(window);
 

 
/* DEMO.CSS */
body
{
 background-color: RGBA(80, 80, 67, .7);
}
img {
 border-style: solid;
 border-width: 4px;
 border-color: black;
 width: 75px;
 height: 75px;
}
a {
 padding-left: 4px;
}
/* END DEMO.CSS */
/* imagefilter.css */
.vexContent {
 width: auto;
 height: auto;
}
#_IFCanvas {
 display: inline-block;
}
.canvas-container {
 margin: auto;
}
.theme-style-none {
 list-style-type: none;
}
/*END IMAGEFILTER.CSS*/
 

<head>
 <link href="http://github.hubspot.com/vex/css/vex.css" rel="stylesheet"/>
 <link href="http://github.hubspot.com/vex/css/vex-theme-os.css" rel="stylesheet"/>
</head>
<body>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
 <script src="http://github.hubspot.com/vex/js/vex.js"></script>
 <script src="http://github.hubspot.com/vex/js/vex.dialog.js"></script>
 <script src="http://fabricjs.com/lib/fabric.js"></script>
 <script>vex.defaultOptions.className = "vex-theme-os";</script>
 <script>
 server = {
 upload: function(){
 console.log(arguments);
 }
 }
 ImageFilter.defaultOptions.imageCallback = server.upload();
 </script>
 <a href="#" data-imagefilter data-imagefilter-image="http://i.imgur.com/uv6T1U2.jpg"><img width="50" height="50" class="example-image" id="imageToEdit" src="http://i.imgur.com/uv6T1U2.jpg" alt="image-1"/></a>
 <a href="#" data-imagefilter data-imagefilter-image="http://i.imgur.com/uv6T1U2.jpg"><img width="50" height="50" class="example-image" id="imageToEdit" src="http://i.imgur.com/uv6T1U2.jpg" alt="image-1" /></a>
</body>

• Roamer-1888: Does this code even run?
• Quill: There's a Stack Exchange bug intefering with my content, try hitting error and looking at it.
• Quill: @Roamer-1888 see my update for that fix

Answer 1

Reusability issues

As it stands, ImageFilter is a strange thing that hovers somewhere between reusable and not reusable.

• Reusable: It is written as a javascript contructor and called with new.
• Not reusable: Only one instance is possible because ImageFilter is a private member of the outer wrapper and only new ImageFilter() is pushed into the global namespace. Once there, in the global namespace, it's not much use; it seems highly unlikely that either of the instance's two methods will ever be called other than by the click handlers that are in place anyway. Also, the instance is tied irrevocably to data-imageFilter-filter elements that exist on page load - no more and no less.

If it is intended that there should only ever be one instance of ImageFilter, then factor it as a module, not a constructor.

If ImageFilter is to be reused, then :

• Export ImageFilter not new ImageFilter(), and allow instances to be created externally, not just one instance internally.
• Don't tie ImageFilter irrevocably to any particular set of elements. Instead, allow click handlers to be attached exterally.

A separate reuse issue concerns everything in the vex dialog (container, canvas, img etc), which is created afresh every time loadImage() is called. Much can be created once per data-imagefilter element (or maybe once in total) and reused by rolling some of loadImage() into the constructor or the outer scope. Leave the resizing and vex.dialog.open() part behind (and all the imgInstance stuff I think).

Also, the this.filters object is static. It need not be defined/redefined in the ImageFilter() constructor.

Global namespace

There's no apparant reason for using the global namespace? If you really want ImageFilter (or an instance) to be globally available, then it could be assigned as a jQuery static method or refactored as a jQuery plugin?

Use of jQuery

Why not exploit the power of jQuery? Coding in POJS is very noble, but bulky and generally less readable.

It's not clear why you might need polyfills for some portions of jQuery when you are already reliant on jQuery for Vex, which is a jQuery plugin.

Others

According to the documentation, fabric.Canvas(); accepts a DOM element, not an id. If the documentation is correct, the canvas element doesn't need an id.

Accessing image.width and image.height immediately after image.src has been set might work in some browsers when an image drawn from cache, but generally not. Best to attach an onload handler and move much of the loadImage() code inside it.

var callback is created conditionally therefore callback is not guaranteed to exist in the line where it is used.

var canvas, self.canvas and this.canvas are confusing.

Why make global an alias for window, then use window?

What is self?

Answer 2

defaultOptions:

Providing a defaultOptions array is useless if you don't use any other property than imageCallback

The following line is redundant if you wish to create imageCallback and not an empty object:

 this.defaultOptions = {};

---

Getting and assigning click listeners:

There's duplicate logic as far as assigning click listeners go, consider moving them to a function:

 $$("[data-imagefilter]", document).forEach(function($el) {
 $el.addEventListener("click", function() {
 global.ImageFilter.loadImage(this.dataset["imagefilterImage"]);
 });
 });

into:

 ImageFilter.prototype.batchAssignClicks = function(selector, callback, datasetSelector){
 $$(selector, document).forEach(function($el) {
 $el.addEventListener("click", function() {
 callback(this.dataset[datasetSelector]);
 });
 });
 }
 ...
 this.batchAssignClicks("[data-imagefilter]", global.ImageFilter.loadImage, "imagefilterImage");

---

self:

Where do the references to self come from?

 self.canvas = new fabric.Canvas(this.CANVAS_ID.replace("#", ""));

Assuming a old self = this trick was tried, the leftover self calls are still present. Remove them.

---

vex.dialog.open:

The first parameter of vex.dialog.open is extraneous, renove it.

---

currentObject.filters:

In the following two lines, an array is reset and then a value added. Why can't we do both?

 currentObject.filters = [];
 currentObject.filters.push(this.filters[filter]);

into:

 currentObject.filters = [this.filters[filter]];

---

Miscellanous

• Name the library something more creative than ImageFilter
• If it's a real library, a minified version of the code should be made too.

Answer 3

Besides all the great points you've made on your answer, you still escaped a very important thing.

On your HTML, you have 2 images.

Both of them have the same id. It is supposed to be unique.

---

On your Javascript side, you are misusing the HTMLElement.dataset property.

On one line, you use this.dataset["imagefilterImage"], while a few lines below you use a.setAttribute('data-imageFilter-filter', filter);.

This shows that you tried to do a.dataset["data-imageFilter-filter"], which failed.

According to the documentation, all property names should be in camelCase, which will be converted to data-small-case-and-dash. Your attempt would return data-image-filter-filter (or similar).

---

Also, did you noticed my 2 examples I've given before?

There are mixed quotes. All over the code!

Please, please, pick a single style and stick with it.

---

Almost at the end, you have this:

(function(global) {
 [...]
 window.onload = function() {
 global.ImageFilter = new ImageFilter();
 };
})(window);

So, what is going on here!?

Why not global.onload, since it is the same as window?

Why pass window as an argument when you are still accessing it in the code?

Change it to gloabl.onload. And just to be sure you have the right window, you can use Function('return this')() instead.

---

Right at the top, there was something that ticked me a bit:

/* polyfills */
var extend = jQuery.extend;

Why don't you just write your own?

Like this:

var extend = function(to_extend) {
 //ignore the first element
 var args = Array.prototype.slice.call(arguments, 1);
 for(var i = 0, l = args.length; i<l; i++)
 {
 for(var k in args[i])
 {
 //we don't want to add things like .toString()
 if(args[i].hasOwnProperty(k))
 {
 //change directly the object, just like jQuery.extend
 to_extend[k] = args[i][k];
 }
 }
 }
 return to_extend;
}

Or just go and copy the source code from jQuery and use it.

Any alternative would be better compared to those 2 lines.

---

And now, to finish it off:

You have a bug!

On your example above, if you decide to do not change it, you are accessing jQuery before you check if it exists.

Try running this:

(function(global){
 var extend = jQuery.extend;
})(window);

This should throw a: ReferenceError: jQuery is not defined. (check your console).

Since you only make your check inside the constructor, it will be pretty useless since the check will never happen.

The code will break long before you get to create ImageFilter.

---

As a bonus, here is a warning:

Be careful with new Image();!

Google Chrome (currently, one of the most used browsers), on version 28, has a bug that would return an object with the wrong prototype.

The fix is easy: Instead of new Image();, use document.createElement('img');.

You can read more about it on: https://stackoverflow.com/a/16378270/2729937

• Roamer-1888: There's no point polyfilling for any part of jQuery. The code uses Vex, which is a jQuery plugin. Therefore without jQuery nothing will work.
• Ismael Miguel: @Roamer-1888 I am aware of that. The "polyfill" (really, it isn't a polyfill at all) is to avoid breaking before it reaches to the part where the code checks if jQuery exists or not. Another option would be to move the check to the top of the code. I will edit this answer in a while, since I've found more issues
• Roamer-1888: Ismael, one thing is for certain, there's no shortage of issues :-)
• Ismael Miguel: @Roamer-1888 There actually is. Quill keeps his code to a high standard. It's really hard to find issues to talk about. When I say "hard", I mean that it is a colossal task to find any issue at all. You have to look to the code from a different perspective. But if you see that there is/are untold issue(s), feel free to answer.
• Roamer-1888: I'll wait to see what else you spot, then make a judgement on whether I need to post or not :-)