Workspace Factory #5: Import Categories, Dropdown UI

[edauterman]

Added feature to allow the user to load a standard Blockly category (logic, loops, math, etc.) by name as a new category. Also changed the add and edit buttons to be dropdowns to (for the add dropdown) add a new category or load a standard category, and (for the edit dropdown) edit the name or the color. Also moved calls to updatePreview to other functions (adding, deleting, editing) and listened for Blockly move and delete events to eliminate the need for the "Update Preview" button (removed button).

---

This change is

[edauterman]

+@picklesrus +@vicng +@quachtina96

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[vicng]

Review status: 0 of 7 files reviewed at latest revision, 11 unresolved discussions.

---

demos/workspacefactory/factory_controller.js, line 379 [r2] (raw file):

    }
  } while (!this.isStandardCategoryName(name));
  // Create an empty category in the model and view.

Nit: Add an extra space above this line. It's easier to read methods in sections, instead of one long paragraph. This paragraph is "where the model is updated".

---

demos/workspacefactory/factory_controller.js, line 384 [r2] (raw file):

  // Load attributes of standard category.
  this.model.loadStandardCategory(id, name);
  // Color the category tab in the view.

Nit: Add extra space above this line. This next paragraph is "where the view is updated".

---

demos/workspacefactory/factory_controller.js, line 404 [r2] (raw file):

 */
FactoryController.prototype.isStandardCategoryName = function(name) {
  for (var category in standardCategories) {

standardCategories should be a property of the Controller (or the Model).

Try to avoid global variables whenever possible.

---

demos/workspacefactory/factory_model.js, line 6 [r2] (raw file):

 * and a unique ID making it possible to change category names and move
 * categories easily. Also keeps track of the currently selected category.
 * Depends on standard_categories.js for standard Blockly categories. .

Nit: Extra period.

---

demos/workspacefactory/factory_model.js, line 255 [r2] (raw file):

 * @param {!string} name The name of the standard category to load.
 */
FactoryModel.prototype.loadStandardCategory = function(id, name) {

This method might be more versatile if it's something like "setCategoryById" (and you worry about loading standard categories inside the controller).

---

demos/workspacefactory/factory_model.js, line 257 [r2] (raw file):

FactoryModel.prototype.loadStandardCategory = function(id, name) {
  var category = this.getCategoryById(id);
  if (!standardCategories[name.toLowerCase()]) {

It's always a bad sign when a method relies on a global variable being defined :)

---

demos/workspacefactory/index.html, line 477 [r2] (raw file):

  var newCategoryWrapper = function() {
    controller.addCategory();
    document.getElementById('dropdown_add').classList.toggle("show");

This should be:

// Hide dropdown menu
...classList.remove("show")

---

demos/workspacefactory/index.html, line 481 [r2] (raw file):

  var loadCategoryWrapper = function() {
    controller.loadCategory();
    document.getElementById('dropdown_add').classList.toggle("show");

Same thing here.

---

demos/workspacefactory/index.html, line 506 [r2] (raw file):

  var nameWrapper = function() {
    controller.changeName();
    document.getElementById('dropdown_edit').classList.toggle("show");

// Hide dropdown menu
classList.remove(...)

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

 */

var standardCategories = Object.create(null);

Add this to a namespace.

---

demos/workspacefactory/standard_categories.js, line 12 [r2] (raw file):

var standardCategories = Object.create(null);

standardCategories['logic'] = {

Are these already defined by the blockly library? If so, try to use those definitions instead of redefining them.

---

Comments from Reviewable

[vicng]

Friendly reminder: try not to add code functionality to a CL after you've submitted it for review. It makes it harder for us to review the CL if it changes from the original submit :)

---

Review status: 0 of 7 files reviewed at latest revision, 11 unresolved discussions.

---

Comments from Reviewable

[picklesrus]

Only 1 nit after what we just discussed. Also, the 2 things this CL does are pretty different (auto update workspace and copy whole category) so could be 2 separate CLS (though don't change that now).

---

Review status: 0 of 7 files reviewed at latest revision, 12 unresolved discussions.

---

demos/workspacefactory/style.css, line 47 [r3] (raw file):

  margin: 0 5px;
  padding: 10px;
  z-index: -1;

how come you had to push this backwards? Is something showing up on top of it?

---

Comments from Reviewable

[edauterman]

Thanks for the feedback -- I'll try to break things up better and not add functionality in the future,

---

Review status: 0 of 7 files reviewed at latest revision, 13 unresolved discussions.

---

demos/workspacefactory/factory_controller.js, line 379 [r2] (raw file):

Previously, vicng wrote…

Nit: Add an extra space above this line. It's easier to read methods in sections, instead of one long paragraph. This paragraph is "where the model is updated".

Added an extra space above. createCategory does work in both the model and the view. Tried to clarify some comments in this method to make them more readable.

---

demos/workspacefactory/factory_controller.js, line 384 [r2] (raw file):

Previously, vicng wrote…

Nit: Add extra space above this line. This next paragraph is "where the view is updated".

switchCategory does work in both the model and view, so I think I have to leave the comment as is.

---

demos/workspacefactory/factory_controller.js, line 404 [r2] (raw file):

Previously, vicng wrote…

standardCategories should be a property of the Controller (or the Model).

Try to avoid global variables whenever possible.

Made it a property of the controller.

---

demos/workspacefactory/factory_model.js, line 6 [r2] (raw file):

Previously, vicng wrote…

Nit: Extra period.

Removed.

---

demos/workspacefactory/factory_model.js, line 255 [r2] (raw file):

Previously, vicng wrote…

This method might be more versatile if it's something like "setCategoryById" (and you worry about loading standard categories inside the controller).

Changed it to be copyCategory where it takes two categories and copies one into the other so that the standard categories are loaded in the controller.

---

demos/workspacefactory/factory_model.js, line 257 [r2] (raw file):

Previously, vicng wrote…

It's always a bad sign when a method relies on a global variable being defined :)

Fixed :)

---

demos/workspacefactory/index.html, line 477 [r2] (raw file):

Previously, vicng wrote…

This should be:

// Hide dropdown menu
...classList.remove("show")

Done.

---

demos/workspacefactory/index.html, line 481 [r2] (raw file):

Previously, vicng wrote…

Same thing here.

Done.

---

demos/workspacefactory/index.html, line 506 [r2] (raw file):

Previously, vicng wrote…

// Hide dropdown menu
classList.remove(...)

Done.

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, vicng wrote…

Add this to a namespace.

Made it a property of the controller.

---

demos/workspacefactory/standard_categories.js, line 12 [r2] (raw file):

Previously, vicng wrote…

Are these already defined by the blockly library? If so, try to use those definitions instead of redefining them.

Not defined by the blockly library, so defining them here.

---

demos/workspacefactory/style.css, line 47 [r3] (raw file):

Previously, picklesrus wrote…

how come you had to push this backwards? Is something showing up on top of it?

There were problems layering with the dropdown and buttons, and I thought I had to push back the buttons and bring forward the dropdown, but just played with it and realized I only need to bring forward the dropdown. Took out the z-index on buttons!

---

Comments from Reviewable

[picklesrus]

Review status: 0 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

demos/workspacefactory/index.html, line 545 [r4] (raw file):

  // Listen for Blockly move and delete events to update preview.
  toolboxWorkspace.addChangeListener(function(e) {
    if (e.type == Blockly.Events.MOVE || e.type == Blockly.Events.DELETE) {

whoops, missed this before. Di d you want to catch the create events too?

---

Comments from Reviewable

[vicng]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/factory_controller.js, line 391 [r4] (raw file):

  this.model.copyCategory(newCategory, standardCategory);
  // Color the category tab in the view.
  if (!newCategory.color) {

Actually, if there's no color, is it ok to set a default one on the category instead?

---

demos/workspacefactory/factory_model.js, line 255 [r2] (raw file):

Previously, evd2014 (Emma Dauterman) wrote…

Changed it to be copyCategory where it takes two categories and copies one into the other so that the standard categories are loaded in the controller.

Cool. In this case, this method should be moved to the Category.

Ideally it would be defined as copy(), which would return a new copy of the existing category (assigned with a new unique id).

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, evd2014 (Emma Dauterman) wrote…

Made it a property of the controller.

I would think that the `prototype` should only be modified by the owning file of `FactoryController` (ie. factory_controller.js). Also, I'm not sure if this should be defined on the prototype, rather than as a global namespaced variable .

Not sure about convention here -- Katelyn, can you clarify?

---

Comments from Reviewable

[picklesrus]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, vicng wrote…

I would think that the prototype should only be modified by the owning file of FactoryController (ie. factory_controller.js). Also, I'm not sure if this should be defined on the prototype, rather than as a global namespaced variable .

Not sure about convention here -- Katelyn, can you clarify?

I had suggested making it part of the prototype, because I thought that meant it would only be created once instead of every time a FactoryController object is created. I'm not really a js expert though... so if you don't think that's the case, then it probably doesn't belong there.

But if you mean for it to be independent of FactoryController all together and just a global variable defined somewhere else, then that's cool too. But I was under the impression we were trying to avoid that.

---

Comments from Reviewable

[vicng]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, picklesrus wrote…

I had suggested making it part of the prototype, because I thought that meant it would only be created once instead of every time a FactoryController object is created. I'm not really a js expert though... so if you don't think that's the case, then it probably doesn't belong there.

But if you mean for it to be independent of FactoryController all together and just a global variable defined somewhere else, then that's cool too. But I was under the impression we were trying to avoid that.

That's fine then. Just wanted clarification since JS is so flexible.

I guess the first rule of JS convention is that there is no convention.

---

Comments from Reviewable

[picklesrus]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, vicng wrote…

That's fine then. Just wanted clarification since JS is so flexible.

I guess the first rule of JS convention is that there is no convention.

Well, but then I just read some more here: http://www.2ality.com/2013/09/data-in-prototypes.html There is some discussion in the comments too... but maybe I was wrong about this.

---

Comments from Reviewable

[edauterman]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/factory_controller.js, line 391 [r4] (raw file):

Previously, vicng wrote…

Actually, if there's no color, is it ok to set a default one on the category instead?

Just changed it so that if it doesn't have a color, then the border color isn't set (which is fine since some categories don't have colors).

---

demos/workspacefactory/factory_model.js, line 255 [r2] (raw file):

Previously, vicng wrote…

Cool. In this case, this method should be moved to the Category.

Ideally it would be defined as copy(), which would return a new copy of the existing category (assigned with a new unique id).

Changed copyCategory to return a new copy of the existing category with a unique ID. I actually chose to not move it to Category because I also need to add it to categoryList.

---

demos/workspacefactory/index.html, line 545 [r4] (raw file):

Previously, picklesrus wrote…

whoops, missed this before. Di d you want to catch the create events too?

I actually tried it with create events too, but because I'm disabling events to update the preview, listening to create events causes the user to "drop" the block right after picking it up. Because moves are fired with creates, I decided to just listen for move and delete to avoid the weird block dropping. Added a comment!

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, picklesrus wrote…

Well, but then I just read some more here: http://www.2ality.com/2013/09/data-in-prototypes.html There is some discussion in the comments too... but maybe I was wrong about this.

Katelyn and I talked about it and there's no functional difference one way or the other because there should only ever be 1 controller, but just to be safe we decided to take it out of prototype.

---

Comments from Reviewable

[edauterman]

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

demos/workspacefactory/standard_categories.js, line 10 [r2] (raw file):

Previously, evd2014 (Emma Dauterman) wrote…

Katelyn and I talked about it and there's no functional difference one way or the other because there should only ever be 1 controller, but just to be safe we decided to take it out of prototype.

Actually realized later that there was a problem with making it not a prototype because then I couldn't define it in a separate file on factory controller. Because controller is a singleton and there's not a danger of it being edited by another object, I changed it back to a prototype.

---

Comments from Reviewable

[picklesrus]

---

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[vicng]

---

Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable