Looking for Some Serious Feedback
New here? Learn about Bountify and follow @bountify to get notified of new bounties! x

I’m not sure if this is an okay thing to request, but I would love feedback on a small app I’ve been tinkering with. It is, I admit, a mess. If someone could please sift through this, make what fixes you can and explain why you did, it would be a great help to me in better understanding how to better develop things like this. My end goal is to eventually rebuild this with something like Parcel but I thought I should ask and learn from feedback before I even begin to tackle that.

I'd like to improve really any part of this, from CSS, to the HTML & in particular the JS. Simplification would be amazing.

I would be willing to tip for follow up solutions to further questions on this. I look forward to your suggestions. Let me know if I can provide any more info.

https://jsfiddle.net/code4ever/dka7x6ec/

Thank you.

Edit: I will be away from my computer but will check into this later tonight or tomorrow morning. Thanks anyone that sends their feedback, I appreciate it.

awarded to CyteBode

Crowdsource coding tasks.

2 Solutions


Hi sharper, I could detect and solve following 4 issues namely

1) Switching button
2) Interior/Exterior button working

3) Restart button working

4) Delete button working

Lastly added few last notes.

For ref only:

Semi-Final JS: https://js.do/code/sharperjs (Truncated after //Bleed area as the sharing platform could not hold it, major changes are included in the snippet)

Final HTML: https://js.do/code/sharperhtml

Final CSS: no changes

Lets begin begin

1) There seemed to be problem switching between buttons. The view would obstruct each other and would not dissmiss on clicking another button.

Cause: In JavaScript, changeView(1) at line #176 is being called before the document is ready.

Solution : Remove changeView(1) at line #176 and add it to line #71 just after canvas2.add(line); ( i.e inside the function at line #62 $(function() {}); )

2) Seems Interior/Exterior call isn't working too

Cause: JS is unable to detect the defined function changeView onclick

Solution: Proceed with this only after completeing 1st solution above.

Step 1) In HTML, change the button at line #9 to: <button id="btnChange" class="btn btn-sm btn-light">

Step 2) In JS, add below code after line #71

$("#btnChange").click(function(){
changeView(1);
});

3) The restart button does not work after applying above solutions

Cause: The definations are expected to be only after the document is ready. Since the doc is not ready the buttons are not really there hence no defination.

Solution: Similar as above, In JS remove the click function at line roughly between #942 - #950 which is below, and add it to the function at $62
$("#restart").click(function() {
if (confirm("This will start you from scratch. Continue?")) {
location.reload();
}
});

4) Lets make delete button work as well

Cause: Same as above

Solution: Same as above. In JS, find (it will be around line #967) and remove below line, add it into the #62 function

$("#delete").click(deleteActiveObject)

Now it will work as desired.

More tips:

1) Looks like fabric initialization has an extra comma (,) at line #168 and #173 initialization of canvas1 and canvas2 respectively

2) Seems there is no need for the parameter value in the changeView() function, you can omit the extra parameter.

I'm pretty sure these problems were caused by sharper hastily copy-pasting their code in a jsfiddle for us to see (how else could the seals (../images/seals/#.jpg) be accessed?). If the code is refactored into separate HTML, JS and CSS files, and the JS code is loaded last, there is no problem.
CyteBode 12 days ago
Hi CyteBode, thanks for adding to it.. As @sharper has asked for feedback for him to better understanding how to better develop things, this is a good opportunity to explan the relationship and respective solution to each one of them. With that I hope it becomes helpful during similar deployments in near future..
SilverHood Apps 12 days ago
Hi SilverHood Apps, thanks so much for taking the time to point out some of those issues. That WAS my bad, I did hastily copy+paste that into JSFiddle; I thought I had used Imgur for all the images, totally spaced there. I appreciate you taking your time to point out those issues. Cheers!
sharper 11 days ago
Cheers 🙂👍 I read there was tip involved for sharing feedback?
SilverHood Apps 11 days ago
That's correct. I will be paid next week and set a reminder to send a tip then!
sharper 11 days ago
Ohkk great sharper thanks 🙂👍
SilverHood Apps 11 days ago
Received, thank you @sharper 👍
SilverHood Apps 8 days ago
@SilverHood Apps, you are very welcome
sharper 7 days ago

The main problem I see is that you've hard-coded the logic over the data, so you're ending up with a ton of duplicated code with slightly different arguments.

The hard-coding approach is usually fine with small prototypes, but it doesn't scale well and quickly goes out of hand as the project gets bigger. It also makes it hard to make changes in logic or to change the data.

The better way to do it is to concisely represent the data in a data structure on its own and run an algorithm over it to apply the logic. For example, the initialization of the click functions for the frames could rather look like this:

var frame_urls = [
  'null',
  'https://i.imgur.com/UgsnnsW.png',
  'https://i.imgur.com/nXNaS1C.png',
  'https://i.imgur.com/ZrUF5BB.png',
  'https://i.imgur.com/0PYOcqD.png',
  'https://i.imgur.com/cxVJX5m.png',
  'https://i.imgur.com/iwok9qK.png',
  'https://i.imgur.com/MKykOxq.png'
];

for (let n in frame_urls) {
    $("#frame" + n).click(function() {
        setBg(frame_urls[n]);
    });
}

More importantly, you should only use the HTML to define the presentation of the data, without including the data itself. The data itself should only exist in a data structure in Javascript and their respective HTML elements added dynamically to the HTML during initialization.

The approach of having data in the HTML and adding logic with JQuery makes sense when the page is being served through CGI, but in a JS-only context, it doesn't work very well.

A good way way to represent the data as a whole, would be as follows:

var items = new List([
  new Category("", [
    new Text("Basic Text", addText),
    new Text("\"In Loving Memory\" (Header)", ilm),
    new Text("\"In Memory of\" (Entire)", si),
    new File("Image from Computer: ", handleImage)
  ]),

  new Category("Frames", [
    new Frame("No Frame", "null"),
    new Frame("Rectangle Mid Tone", "https://i.imgur.com/UgsnnsW.png"),
    ...
  ]),

  new Category("Themes and Seals", [
    new SubCategory("Military", [
      new Theme("Airforce", "https://i.imgur.com/3no2zWB.png"),
      ...
      new Seal("Department of Defense", "../images/seals/1.jpg"),
      ...
    ]),
    new SubCategory("Food", [
      new Theme("Baking", "https://i.imgur.com/9BH6ZHW.png"),
      ...
    ]),
    ...
  ]),

  new Category("Verses", [
    new Verse("Another Leaf Has Fallen",
          "Another leaf has fallen,\n" +
          "another soul has gone.\n" +
          "But still we have God's promises,\n" +
          "in every robin's song.\n" +
          "For he is in His heaven,\n" +
          "and though He takes away,\n" +
          "He always leaves to mortals,\n" +
          "the bright sun's kindly ray.\n" +
          "He leaves the fragrant blossoms,\n" +
          "and lovely forest, green.\n" +
          "And gives us new found comfort,\n" +
          "when we on Him will lean."),
    ...
  ])
]);

Where the List, Text, File, etc. classes are defined as follows:

class List {
  constructor(list) {
    this.list = list;
  }

  addToHTML() {
    var ul = $("#add-menu");
    for (let n in this.list) {
      let elm = this.list[n];
      elm.addToHTML();
      if (n < this.list.length - 1) {
        let divider = $("<div></div>");
        divider.addClass("dropdown-divider");
        ul.append(divider);
      }
    }
  }
}

class Category {
  constructor(name, lst, tag) {
    if (typeof tag === "undefined") {
      tag = "h5";
    }
    this.name = name;
    this.lst = lst;
    this.tag = tag;
  }

  addToHTML() {
    var ul = $("#add-menu");
    if (this.name) {
      let startTag = "<" + this.tag + ">";
      let endTag = "</" + this.tag + ">";
      let header = $(startTag + this.name + endTag);
      header.addClass("dropdown-header");
      ul.append(header);
    }

    for (let elm of this.lst) {
      elm.addToHTML();
    }
  }
}

class SubCategory extends Category {
  constructor(name, lst) {
    super(name, lst, "h6");
  }
}

class NamedItem {
  constructor(name, search) {
    this.name = name;
    this.search = search;
  }

  addToHTML() {
    var element = $("<a></a>");
    if (this.search) {
      let searchItem = $("<span>" + this.search + "</span>");
      searchItem.addClass("searchitem");
      element.append(searchItem);
    }
    element.attr("href", "#");
    element.append(document.createTextNode(this.name));
    element.addClass("dropdown-item");
    $("#add-menu").append(element);
    return element;
  }
}

class File extends NamedItem {
  constructor(name, fn) {
    super(name);
    this.fn = fn;
  }

  addToHTML() {
    var element = super.addToHTML();
    element.change(this.fn);
    var input = $("<input type=\"file\" accept=\"image/*\" multiple/>");
    element.append(input);
  }
}

class ClickableNamedItem extends NamedItem {
  constructor(name, fn, search) {
    super(name, search);
    this.fn = fn;
  }

  addToHTML() {
    super.addToHTML().click(this.fn);
  }
}

class Text extends ClickableNamedItem {
  constructor(name, fn) {
    super(name, fn);
  }
}

class Background extends ClickableNamedItem {
  constructor(name, url) {
    super(name, function() {
      // TODO
    }, "background");
  }
}

class Frame extends ClickableNamedItem {
  constructor(name, url) {
    super(name, function() {
      setFrame(url);
    }, "frames");
  }
}

class Theme extends ClickableNamedItem {
  constructor(name, url) {
    super(name, function() {
      setAccent(url);
    }, "themes");
  }
}

class Seal extends ClickableNamedItem {
  constructor(name, url) {
    super(name, function() {
      fabric.Image.fromURL(url, function (oImg) {
        oImg.scaleToHeight(100);
        mainCanvas.add(oImg);
      });
    }, "military seals");
  }
}

And that part in the HTML is essentially left empty, but it is filled by calling items.addToHTML():

<ul class="dropdown-menu scrollable-menu">
  <input class="form-control" id="search-criteria" type="text" placeholder="Search...">
  <!-- Where the items will go -->
</ul>

Now, it becomes trivial to make changes to the data (such as adding new frames or even categories in the verses) without having to change anything in more than one place.

I have integrated the solution in your project. Thanks to my improvement, the javascript was reduced by 400 lines and the HTML by 150. I had to modify the rest of the script and the HTML a bit, so here are the files:

Edit 1: Added proposed data structure.

Edit 2: Added implementation of the proposed improvement and integration.

Hi there! This is very interesting. I'm going to tinker with this today. Would you be interested in possibly writing what you can from the JSFiddle in the manner that you described for a tip of the amount of the bounty? Thank you so much for taking the time to explain the above. It's very helpful. Cheers!
sharper 11 days ago
Thanks for awarding me the bounty. I've modified the solution to include the implementation of the classes and the integration into your project.
CyteBode 11 days ago
CyteBode, this is really cool! Thank you again. I checked it out and ma going to tinker with it tonight to try and better familiarize myself with it. I can see how this would definitely simplify expanding the app. I will send the tip within the next few days, once I am paid. After that, would you be interested in some follow up stuff? I'd love to incorporate this into some other work. Cheers!
sharper 11 days ago
As a quick follow-up question, why is line 455 of the data.js not formatted in my text editor ever? Wherever that chunk is, it seems to not format and I've always wondered why! Haha. What do you think?
sharper 11 days ago
It's either because of the (U+201D) characters, which are "right double quotation marks" instead of normal quotation marks, or because the line is just too long (it's the longest one after all). The syntax highlighting works fine in Sublime Text.
CyteBode 11 days ago
That makes sense. Hey, thanks again!
sharper 11 days ago
View Timeline