968808 - (one-click-new-app) Implement a "Create New App" button in the App Manager to create new apps from templates in one click

Status: RESOLVED WORKSFORME

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Jan Keromnes [:janx]]

Let's implement a preffed-off prototype "Create New App" button that lets you choose:

- an app name,
- an app template from a list (e.g. http://people.mozilla.org/~spenades/mortar_devtools/list.json),

creating a new App Manager project for you.

Comment 2

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=paul

Comment 3

[Jan Keromnes [:janx]]

WIP, sort-of works but several TODOs and FIXMEs left.

Paul, what's the best way to use the AppManager Lense to ask the user for project name and desired template?

Comment 4

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=paul

Comment 5

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=paul

Comment 6

[Jan Keromnes [:janx]]

Attachment: create_new_app.png

Here is a screenshot of the "Create New App" screen. It downloads the template list from http://people.mozilla.org/~spenades/mortar_devtools/list.json

TODO:
- Add an "icon" field with an image url in the JSON
- Fix the TODOs left in the code

Comment 7

[Soledad Penades [:sole] [:spenades]]

Icon field with image url in the JSON == DONE :-)

Comment 8

[Jan Keromnes [:janx]]

Thanks Sole! Are both icons just white rectangles?

Comment 9

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=paul

Comment 10

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=paul

Comment 11

[Jan Keromnes [:janx]]

Comment on attachment 8378472 [details] [diff] [review]
Implement a "Create New App" button in the App Manager. r=paul

Paul, care to take a look?

Comment 12

[Paul Rouget [:paul]]

Comment on attachment 8378472 [details] [diff] [review]
Implement a "Create New App" button in the App Manager. r=paul

>+  _placeholder: {
>+    name: "",
>+    description: "downloading...",
>+    // Smallest possible transparent gif (TODO: spinner?).
>+    icon: "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="
>+  },

This is hacky, but it will work. You'll need to extract "downloading…" and put it in a .properties file.
And use the profiler spinner:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/cleopatra/images/throbber.svg

>+  // FIXME pref off app creator

FIXIT.

>+  showAppCreator: function() {
>+    document.body.classList.add("creating");
>+    let boilerplates = this.store.object.boilerplates;
>+
>+    boilerplates.splice(0);
>+    boilerplates.push(this._placeholder);
>+
>+    // Download JSON list of boilerplates
>+    let xhr = new XMLHttpRequest();
>+    xhr.onload = function() {
>+      boilerplates.splice(0);
>+      let list = JSON.parse(this.responseText);
>+      for (let boilerplate of list) {
>+        boilerplates.push(boilerplate);
>+      }
>+    }
>+    xhr.open("get", "http://people.mozilla.org/~spenades/mortar_devtools/list.json", true);
>+    xhr.send();
>+  },


What if 404 or connection error?
Also, store this URL in a pref.

>+  clearAppCreator: function() {
>+    document.body.classList.remove("creating");
>+    document.querySelector(".name-input").value = "";
>+    this.selectBoilerplate();
>+  },

PAUL: when is that called?

>+
>+  selectBoilerplate: function(element) {
>+    let list = document.querySelector(".boilerplate-list");
>+    let selected = document.querySelector(".boilerplate-item.selected");
>+
>+    list.classList.remove("focus");

A focus class? What about the actual focus pseudo class?

>+    if (selected) {
>+      selected.classList.remove("selected");
>+    }

You might want to use a radio button. Then you won't have to implement your own selection mechanism.

>+    if (element && element.dataset.url && element.dataset.url != "n/a") {
>+      element.classList.add("selected");
>+    }
>+  },

When is url == "n/a"?
Also, don't trust "n/a", we might change this value later.


>+    // Select new app folder
>+    let folder = UI._selectFolder();
>+    if (!folder) return;
>+
>+    // Create subfolder with fs-friendly name of project
>+    let name = input.value;
>+    let subfolder = name.replace(/\W/g, '').toLowerCase();
>+    folder.append(subfolder);
>+    folder.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);

You'll need to test that on the 3 platforms.


>+  // TODO refactor to also allow zip filepath / url with code above

DOIT?

>--- a/browser/devtools/app-manager/content/projects.xhtml

>+  <template id="boilerplate-item-template">
>+    <li class="boilerplate-item" onclick="UI.selectBoilerplate(this)" template='{"type":"attribute","name":"data-url","path":"file"}'>
>+      <img class="boilerplate-item-icon" template='{"type":"attribute","name":"src","path":"icon"}' />
>+      <div class="boilerplate-item-meta">
>+        <strong template='{"type":"textContent","path":"name"}' />
>+        <p class="boilerplate-item-description" template='{"type":"textContent","path":"description"}' />
>+      </div>
>+    </li>
>+  </template>


This is where you want to use radio buttons.

>+<!ENTITY projects.createNewApp "Create New App">
>+<!ENTITY projects.createNewAppTooltip "Create a new app from a boilerplate">

s/boilerplate/template/

Comment 13

[Daniele "Mte90" Scasciafratte]

Why not add the Firefox OS Boilerplate App in the list?
https://github.com/robnyman/Firefox-OS-Boilerplate-App
In the hackaton always recommend this.

Comment 14

[Jan Keromnes [:janx]]

Thanks for your suggestion, Mte90! Sole, care to take a look?

Comment 15

[Soledad Penades [:sole] [:spenades]]

Hey Daniele, thanks for the idea

I think for now we are going to focus on smaller app templates that demonstrate a minimal set of related functional features, not a huge amount of API demonstrations as the boilerplate does.

This is subject to change, of course. I'll keep this idea in mind.

Also in the future there shouldn't be a "list" and maybe you'll get the templates from some sort of repository where everyone can contribute. Or from any website! This list is temporary.

Comment 16

[Daniele "Mte90" Scasciafratte]

In the talks i show the boilerplate with all the examples because it's a demo of the features for the developers.
I think that a developer prefer a boilerplate that a mini template.

Comment 17

[Soledad Penades [:sole] [:spenades]]

Hey Daniele

I've created an issue for this in the project that generates the list: https://github.com/sole/mortar-devtools/issues/3

so we can keep this bug on topic--this is for "implementing the new app button", not for deciding which templates go in the list :-)

Comment 18

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=jryans

Comment 19

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=jryans

Comment 20

[Jan Keromnes [:janx]]

Attachment: Implement a "Create New App" button in the App Manager. r=jryans

Comment 21

[Jan Keromnes [:janx]]

Comment on attachment 8393018 [details] [diff] [review]
Implement a "Create New App" button in the App Manager. r=jryans

Review of attachment 8393018 [details] [diff] [review]:
-----------------------------------------------------------------

Addressed almost all of Paul's nits, except the request to use radio buttons for the template list, because I don't like radio buttons that much.

Ryan, please take a look. I might yield on the radio buttons if you insist.

::: browser/app/profile/firefox.js
@@ +1105,5 @@
>  
>  // Enable the app manager
>  pref("devtools.appmanager.enabled", true);
>  pref("devtools.appmanager.lastTab", "help");
> +pref("devtools.appmanager.appCreator.enabled", true);

This is just `true` for convenience. I'll flip it to `false` before landing.

::: browser/themes/shared/devtools/app-manager/projects.css
@@ -534,5 @@
>    border: 0;
>    border-top: 5px solid #C9C9C9;
>  }
>  
> -/* Bug 925921: Remove when the manifest editor is always on */

I took the liberty to remove this because the bug was fixed 3 months ago.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8393018 [details] [diff] [review]
Implement a "Create New App" button in the App Manager. r=jryans

Review of attachment 8393018 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be coming along well! :) I like the functionality overall.  More specific comments:

Sometimes they are "boilerplates", sometimes "templates".  Let's avoid "boilerplate" everywhere, as none of the discussion I can see (in bugs) refers to them that way.  It's generally better to avoid making up new names for things in code (that differ from the names people use to discuss a feature) when possible, since that makes it harder to search for the relevant code later on.  In JS / HTML / CSS, let's call them "app templates" (since you need to differentiate between variables for the page's template, usually just named "template", which would get confusing real fast) or just "templates" if that doesn't seem to conflict.

I noticed that if you have an existing project selected when you click "Create New App", the existing project remains selected, even though it's hidden.  It should probably be deselected while the creator is visible.  If the creator is canceled, re-select the previously selected project.

::: browser/app/profile/firefox.js
@@ +1106,5 @@
>  // Enable the app manager
>  pref("devtools.appmanager.enabled", true);
>  pref("devtools.appmanager.lastTab", "help");
> +pref("devtools.appmanager.appCreator.enabled", true);
> +pref("devtools.appmanager.appCreator.templatesURL", "http://people.mozilla.org/~spenades/mortar_devtools/list.json");

people.m.o is not meant to be used for production content.  I guess the plans to move this somewhere else are still under discussion in bug 980085.  Please file a bug for enabling this feature by default (with the "dev-doc-needed, feature" keywords) and make it clear that we can't enable by default without first moving this somewhere production-ready.

Also, can this list contain more than one entry?  It would aid testing of selection edge cases, etc.

::: browser/devtools/app-manager/content/projects.js
@@ +62,5 @@
>        document.querySelector("#lense").setAttribute("manifest-editable", "");
>      }
>  
> +    if (Services.prefs.getBoolPref(APP_CREATOR_ENABLED)) {
> +      document.querySelector("#sidebar").classList.add("app-creator");

"app-creator" doesn't describe a state.  It makes me think the creator is currently active, but that's not what this means.  Use "app-creatable".  Also, use an attribute as above (Paul seemed to prefer them for this type of thing).

@@ +66,5 @@
> +      document.querySelector("#sidebar").classList.add("app-creator");
> +    }
> +
> +    this.store = Utils.mergeStores({
> +      "projects": AppProjects.store,

I think if you make this "AppProjects.store.projects" you can drop the "projects.projects" back down to just "projects" in the templates.

@@ +67,5 @@
> +    }
> +
> +    this.store = Utils.mergeStores({
> +      "projects": AppProjects.store,
> +      "boilerplates": new ObservableObject([])

Currently we're mixing in the logic to manage the template list (the model) with its supporting UI (the view / controller) in this one file.  Plus, there's already quite a lot in this one file.

You should create an AppTemplates store in a separate file that does the list management only (retrieves the list, etc.)  There are quite a few other "store" things in the App Manager to give you ideas on structure.  This projects.js file itself would then consume the new store.

@@ +127,5 @@
> +    document.body.classList.add("creating");
> +    let boilerplates = this.store.object.boilerplates;
> +
> +    boilerplates.splice(0);
> +    boilerplates.push(this._placeholder);

Inline this object here.  Also, you probably only need the "icon" property (I hope).  Make a const for the path to icon.

@@ +146,5 @@
> +  /**
> +   * This method cleans up and hides the app creator panel, when an app has successfully been
> +   * created, or the user has cancelled the app creation.
> +   */
> +  clearAppCreator: function() {

clear -> close

@@ +152,5 @@
> +    document.querySelector(".name-input").value = "";
> +    this.selectBoilerplate();
> +  },
> +
> +  selectBoilerplate: function(element) {

You could still use radio buttons...  I don't think Paul necessarily meant for the actual radio button UI control to be shown.  You can tie your current UI to a hidden radio button so that clicking your UI selects the radio button via the label element.  Then the selection is managed for you.

@@ +170,5 @@
> +    let input = document.querySelector(".name-input");
> +    if (!input.value) {
> +      // Abort and ask for project name
> +      input.value = input.placeholder;
> +      input.select();

This is pretty subtle...  Might be better to actually display error on-screen of "please enter a name", similar to the way validation errors are displayed.  But, it seems okay to do this later in separate bug (please file if so).

@@ +176,5 @@
> +    }
> +
> +    let boilerplate = document.querySelector(".boilerplate-item.selected");
> +    if (!boilerplate) {
> +      document.querySelector(".boilerplate-list").classList.add("highlighted");

This is similarly quite subtle and could use an actual message.

@@ +179,5 @@
> +    if (!boilerplate) {
> +      document.querySelector(".boilerplate-list").classList.add("highlighted");
> +      return;
> +    }
> +    console.log(boilerplate);

Remove.

@@ +182,5 @@
> +    }
> +    console.log(boilerplate);
> +
> +    // Select new app folder
> +    let folder = UI._selectFolder();

Reusing this is misleading, because the dialog says "Select a webapp folder", but really you are choosing a root "projects" folder, in which you are about to put a new folder for the app itself.  Maybe refactor this so you can supply your own dialog title.

@@ +183,5 @@
> +    console.log(boilerplate);
> +
> +    // Select new app folder
> +    let folder = UI._selectFolder();
> +    if (!folder) return;

Braces, new line.

@@ +187,5 @@
> +    if (!folder) return;
> +
> +    // Create subfolder with fs-friendly name of project
> +    let name = input.value;
> +    let subfolder = name.replace(/\W/g, '').toLowerCase();

Convert spaces to hyphens or some separator, rather than dropping them.

@@ +202,5 @@
> +      target.remove(false);
> +      this.clearAppCreator();
> +      UI.addPackaged(folder);
> +      this.once("project-selected", () => {
> +        let m = this.manifestEditor;

Use a real word.

@@ +208,5 @@
> +        m.update();
> +        m.save();
> +      });
> +    }, (err) => {
> +      console.error(err);

Don't need a wrapping function here.

We should display some kind of user visible error here, instead of seeming to do nothing.  If the creator is still visible at error time, a message could be show in there (like the validation errors).

::: browser/devtools/app-manager/content/projects.xhtml
@@ +27,3 @@
>          <div id="no-project">&projects.noProjects;</div>
>        </div>
> +      <div id="new-project" onclick="UI.showAppCreator()" title="&projects.createNewAppTooltip;">&projects.createNewApp;</div>

"new-project" is too general given the two new-*-project names below.  Can't think of a suggestion at the moment though... :)

@@ +37,5 @@
>        </div>
>      </aside>
>      <section id="lense"></section>
> +    <section id="creator">
> +      <h1>Create a new App</h1>

There should be very little / no actual text in HTML files.  All these strings need to go into the DTD so they can be translated (into wacky things like "créer une nouvelle app"). ;)

@@ +39,5 @@
>      <section id="lense"></section>
> +    <section id="creator">
> +      <h1>Create a new App</h1>
> +      <h2>Name</h2>
> +      <input class="name-input" type="text" placeholder="App name" />

"-input" is redundant, drop it.  You can use #creator .name or input.name to select it in CSS.

@@ +44,5 @@
> +      <h2>Template</h2>
> +      <ul class="boilerplate-list" template-loop='{"arrayPath":"boilerplates","childSelector":"#boilerplate-item-template"}'>
> +      </ul>
> +      <div class="creator-buttons">
> +        <button class="button-create" onclick="UI.createNewApp()">Create</button>

Seems fine to drop the "button-" prefix.

::: browser/themes/linux/jar.mn
@@ +281,5 @@
>    skin/classic/browser/devtools/app-manager/index-icons.svg           (../shared/devtools/app-manager/images/index-icons.svg)
>    skin/classic/browser/devtools/app-manager/rocket.svg                (../shared/devtools/app-manager/images/rocket.svg)
>    skin/classic/browser/devtools/app-manager/noise.png                 (../shared/devtools/app-manager/images/noise.png)
>    skin/classic/browser/devtools/app-manager/default-app-icon.png      (../shared/devtools/app-manager/images/default-app-icon.png)
> +  skin/classic/browser/devtools/app-manager/throbber.svg              (../shared/devtools/app-manager/images/throbber.svg)

You'll need to update OS X and Windows.  The Windows file has 2 entries for each thing as well, so be careful there.

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +32,5 @@
> +body:not(.creating) #creator {
> +  display: none;
> +}
> +
> +body.creating #lense {

Let's use a different class for this state so that we can also control #lense the same way:

body:not(.viewing) #lense

@@ +86,5 @@
>    display: flex;
>    position: relative;
>  }
>  
> +.project-item:hover, .boilerplate-item:hover {

In general, follow our CSS style guide[1].  Note that the "Content or Theme" section does not apply to the App Manager.

In this case (and other below), each selector goes on its own line.

[1]: https://wiki.mozilla.org/DevTools/CSSTips

@@ -534,5 @@
>    border: 0;
>    border-top: 5px solid #C9C9C9;
>  }
>  
> -/* Bug 925921: Remove when the manifest editor is always on */

Actually, we should do this removal at the same time that we kill the pref to control it.  So, don't do this here.  I filed bug 985287 to do this work.

@@ +563,4 @@
>    flex-grow: 1;
> +  flex-direction: column;
> +  overflow: hidden;
> +  background-color: whitesmoke;

It's a bit weird to me that we've got a mixture of named and numeric color values in here...

Anyway, use the same background color as the lense.

@@ +619,5 @@
> +  color: #FFF;
> +}
> +
> +.button-cancel {
> +  color: red;

Try out the wacky orange / pink color we use for the "remove a project" button.

@@ +623,5 @@
> +  color: red;
> +}
> +
> +.button-cancel:hover {
> +  background-color: red;

Same here.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Also, we need a test for this.  Try looking at manifest editor for some ideas.

Comment 24

[Soledad Penades [:sole] [:spenades]]

(In reply to J. Ryan Stinnett [:jryans] from comment #22)
 
> Sometimes they are "boilerplates", sometimes "templates".  Let's avoid
> "boilerplate" everywhere, as none of the discussion I can see (in bugs)
> refers to them that way.  It's generally better to avoid making up new names
> for things in code (that differ from the names people use to discuss a
> feature) when possible, since that makes it harder to search for the
> relevant code later on.  In JS / HTML / CSS, let's call them "app templates"
> (since you need to differentiate between variables for the page's template,
> usually just named "template", which would get confusing real fast) or just
> "templates" if that doesn't seem to conflict.

We're just calling them "templates" - or "app templates" if there is potential for confusion (e.g. if talking about the <template> element in the same conversation). Definitely please don't use "boilerplate" :-)

> > +pref("devtools.appmanager.appCreator.templatesURL", "http://people.mozilla.org/~spenades/mortar_devtools/list.json");
> 
> people.m.o is not meant to be used for production content.  I guess the
> plans to move this somewhere else are still under discussion in bug 980085.

Yes, we're working on it! We want to get a CDN subdomain for our team (apps) so that we can host this kind of content there, instead of using p.m.o or github. See https://bugzilla.mozilla.org/show_bug.cgi?id=981801 for more info (should I mark this bug as depending on that one? it seemed overkill to me but you know better than me)

> Also, can this list contain more than one entry?  It would aid testing of
> selection edge cases, etc.

It can, but I haven't had time for writing more templates. Since this is preffed and it's just pointing to a JSON file, you could very well just duplicate/triplicate/quadruplicate/etc the existing entry and upload it somewhere else to test the case of having more than one entry.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Soledad Penades [:sole] [:spenades] from comment #24)
> (In reply to J. Ryan Stinnett [:jryans] from comment #22)
> > people.m.o is not meant to be used for production content.  I guess the
> > plans to move this somewhere else are still under discussion in bug 980085.
> 
> Yes, we're working on it! We want to get a CDN subdomain for our team (apps)
> so that we can host this kind of content there, instead of using p.m.o or
> github. See https://bugzilla.mozilla.org/show_bug.cgi?id=981801 for more
> info (should I mark this bug as depending on that one? it seemed overkill to
> me but you know better than me)

Ah great, hadn't seen that one yet!  Let's have Jan file the "enabled by default" bug I mentioned above, and that new bug should depend on this current bug and the one you mentioned here.

Comment 26

[Jan Keromnes [:janx]]

Ryan, thank you so much for the excellent feedback!

Paul and I felt it makes more sense to introduce this feature directly with the App Manager v2, because shipping a temporary preffed-off version doesn't really make sense, and the templates are not quite ready yet. We've reviewed your suggestions, and taken them into account when Paul migrated the feature to his appmgr_v2 branch.

Comment 27

[Paul Rouget [:paul]]

Code has landed in the new UI.