feat(plnkr): Add initial version of openPlunker #26

tinayuangao · 2016-11-18T19:09:37Z

mmalerba · 2016-11-18T19:45:35Z

src/app/shared/plnkr/plnkr-util.ts

+
+  TAGS: string[] = ['angular', 'material', 'example'];
+
+  description: string = 'Example for Angular Material';


Maybe add all of these to some kind of ExampleDescriptor class?

mmalerba · 2016-11-18T19:47:19Z

src/app/shared/plnkr/plnkr.ts

+import { PlunkerUtil } from './plnkr-util';
+
+@Component({
+  selector: 'app-plnkr',


Maybe "plunker-example"? (also be consistent with plnkr vs plunker, my vote is for everything to be "plunker")

mmalerba · 2016-11-18T19:48:23Z

src/app/shared/plnkr/example/button-demo.html

@@ -0,0 +1,104 @@
+<div class="demo-button">


I know these will eventually go in a separate npm package, but maybe for now put them in assets/examples?

mmalerba · 2016-11-18T19:49:13Z

src/app/shared/plnkr/template/index.html

@@ -0,0 +1,36 @@
+<!DOCTYPE html>


can go in assets/plunker

I still advocate moving the template files to assets, they're not really part of the application source

jelbourn · 2016-11-18T19:21:47Z

src/app/shared/plnkr/plnkr.ts

+  selector: 'app-plnkr',
+  templateUrl: 'plnkr.html',
+})
+export class Plunker implements OnInit {


How about calling this PlunkerButton with selector plunker-button?

jelbourn · 2016-11-18T19:27:26Z

src/app/shared/plnkr/plnkr-util.ts

+    can be found in the LICENSE file at http://angular.io/license`;
+
+  TEMPLATE_PATH = '/app/shared/plnkr/template/';
+  TEMPLATE_FILES = ['index.html', 'systemjs.config.js', 'main.ts'];


We can move these constants outside the class since they're effectively private. That way we don't have to refer to them with this

jelbourn · 2016-11-18T19:29:24Z

src/app/shared/plnkr/plnkr-util.ts

@@ -0,0 +1,109 @@
+import { Http } from '@angular/http';
+
+export class PlunkerUtil {


How about something like PlunkerWriter, PlunkerExporter, or PlunkerConnector?

cc @mmalerba

jelbourn · 2016-11-18T19:35:23Z

src/app/shared/plnkr/plnkr-util.ts

+  exampleFiles: string[] = ['button-demo.html', 'button-demo.scss', 'button-demo.ts'];
+  selectorName: string = 'button-demo';
+  indexFilename: string = 'button-demo';
+  componentName: string = 'ButtonDemo';


Would it make sense to have a separate PlunkerData class that you construct and pass around rather than storing all of these properties directly on the service?

jelbourn · 2016-11-18T19:44:05Z

src/app/shared/plnkr/plnkr-util.ts

@@ -0,0 +1,109 @@
+import { Http } from '@angular/http';
+


Can you add a top-level comment here that gives an explanation of the Plunker API since it doesn't seem to be documented anywhere on their site?

jelbourn · 2016-11-18T19:47:44Z

src/app/shared/plnkr/plnkr-util.ts

+    return form;
+  }
+
+  _generateInput(name: string, value: string, isFile = false) {


I'd call this _createFormInput

jelbourn · 2016-11-18T19:49:44Z

src/app/shared/plnkr/plnkr-util.ts

+    if (filename.indexOf('.ts') > -1 || filename.indexOf('.scss') > -1) {
+      content = content + '/** ' + this.COPYRIGHT + ' */';
+    } else if (filename.indexOf('.html') > -1) {
+      content = content + '<!-- ' + this.COPYRIGHT + ' -->';


Use a template string for these:

`${content}\n\n/** ${COPYRIGHT} */` `${content}\n\n`

jelbourn · 2016-11-18T19:50:48Z

src/app/shared/plnkr/plnkr-util.ts

+      }
+    }
+
+    if (filename.indexOf('.ts') > -1 || filename.indexOf('.scss') > -1) {


I would break this out into its own method _appendCopyright

jelbourn · 2016-11-18T19:51:42Z

src/app/shared/plnkr/plnkr-util.ts

+  }
+
+  _processContent(filename: string, path: string, content: string): string {
+    if (path == this.TEMPLATE_PATH) {


I don't quite follow what these replacements are for.

Added comments

jelbourn · 2016-11-18T20:00:02Z

src/app/shared/plnkr/plnkr-util.ts

+      (response) => {
+        let content = response.text();
+        content = this._processContent(filename, path, content);
+        this._generateInput(filename, content, true);


I'd break these out into separate handler functions:

this._http.get(path + filename).subscribe( response => this._addFileToForm(response.text()), error => console.log(error));

Where _addFileToForm massages the file content and adds the input. Then you could remove the isFile argument from _createFormInput and pass the files[${name}] as the name (since you know specifically you're processing a file).

tinayuangao · 2016-11-19T01:04:01Z

Added PlunkerData and changed PlunkerUtil to PlunkerWriter. PTAL. Thanks for review!

jelbourn · 2016-11-19T03:06:50Z

src/app/shared/plnkr/plunker-writer.ts

+ *   private: true
+ * }
+ */
+export class PlunkerWriter {


You can add the @Injectable decorator to this to turn it into a service and add it to the providers list of the app's NgModule. Then PlunkerButton can inject this (instead of http) and this service can inject Http directly.

jelbourn · 2016-11-19T03:08:44Z

src/app/shared/plnkr/plunker-writer.ts

+  TEMPLATE_PATH = '/app/shared/plnkr/template/';
+  TEMPLATE_FILES = ['index.html', 'systemjs.config.js', 'main.ts'];
+
+  TAGS: string[] = ['angular', 'material', 'example'];


I think you missed moving these constants out of the class; they can go right above it (example)

jelbourn · 2016-11-19T03:10:01Z

src/app/shared/plnkr/plunker-writer.ts

+      content = this._processTemplateFile(filename, path, content);
+    }
+    this._createFormInput(`files[${filename}]`, this._appendCopyright(filename, content));
+    this._submitForm();


Did you mean to move this _submitForm call to the end of openPlunker?

jelbourn · 2016-11-19T03:29:16Z

src/app/shared/plnkr/plunker-writer.ts

+      // For example, <material-docs-example></material-docs-exmaple> will be replaced as
+      // <button-demo></button-demo>
+      content = content.replace(new RegExp('material-docs-example', 'g'),
+        this.exampleData.selectorName);


You could use a regex literal:

content = content.replace(/material-docs-example/g, this.exampleData.selectorName),

jelbourn · 2016-11-19T03:32:42Z

src/app/shared/plnkr/plunker-writer.ts

+    this._submitForm();
+  }
+
+  _processTemplateFile(filename: string, path: string, content: string): string {


How about

/** * The Plunker template assets contain placeholder names for the examples: * "<material-docs-example>" and "MaterialDocsExample". * This will replace those placeholders with the names from the example metadata, * e.g. "<basic-button-example>" and "BasicButtonExample" */ _replaceExamplePlaceholderNames(fileName: string, fileContent: string) { ... }

I also noticed that the path argument seems to not be used in this function.

tinayuangao · 2016-11-21T20:07:08Z

Comments addressed. Please take another look. Thanks!

mmalerba · 2016-11-21T20:23:08Z

src/app/shared/plnkr/index.ts

@@ -0,0 +1 @@
+export * from './plunker-button';


Change the directory name to plunker

mmalerba · 2016-11-21T20:24:20Z

src/app/shared/plnkr/plunker-data.ts

+ * Plunker data
+ *   with information about Component name, selector, files used in example, and path to examples
+ */
+export class PlunkerData {


I think this is fine for this CL, but this stuff isn't actually related to plunker at all, it should be called ExampleData and should probably live in the material2 repo eventually.

Changed the name to ExampleData. I think we can extract information (selector, componentName) from example file. I will work on that in another PR.

mmalerba · 2016-11-21T20:26:25Z

src/app/shared/plnkr/template/index.html

@@ -0,0 +1,36 @@
+<!DOCTYPE html>


I still advocate moving the template files to assets, they're not really part of the application source

tinayuangao

Thanks for review. Moved template files to assets/plunker.

tinayuangao · 2016-11-21T21:29:09Z

src/app/shared/plnkr/plunker-data.ts

+ * Plunker data
+ *   with information about Component name, selector, files used in example, and path to examples
+ */
+export class PlunkerData {


Changed the name to ExampleData. I think we can extract information (selector, componentName) from example file. I will work on that in another PR.

tinayuangao · 2016-12-19T22:15:46Z

Added test for plunker-writer

jelbourn · 2016-12-21T01:47:32Z

LGTM

jelbourn · 2016-12-21T01:48:36Z

Actually, I just noticed that NavBar is now declared in two modules; it should be removed from AppModule

tinayuangao · 2016-12-21T20:12:37Z

Updated - fixed NavBar and integrated with example view. Moved example-data to a better place

jelbourn · 2016-12-21T20:29:26Z

LGTM, we can update to use the real examples in a follow-up PR.

googlebot added the cla: yes label Nov 18, 2016

mmalerba reviewed Nov 18, 2016

View reviewed changes

jelbourn reviewed Nov 18, 2016

View reviewed changes

jelbourn reviewed Nov 19, 2016

View reviewed changes

mmalerba reviewed Nov 21, 2016

View reviewed changes

tinayuangao commented Nov 21, 2016

View reviewed changes

mmalerba approved these changes Nov 21, 2016

View reviewed changes

tinayuangao force-pushed the plunker branch 2 times, most recently from 5884c5b to 0f81773 Compare December 19, 2016 22:15

tinayuangao added 6 commits December 20, 2016 10:47

Add initial version of openPlunker

17bcae3

Add PlunkerData

3a75bf5

Make openPlunker work

df37ecb

Move plunker templates location and example data name

e901080

Move plunker template to assets/plunker

213537d

Add test

299c403

tinayuangao force-pushed the plunker branch from 48b7a1b to f3974f5 Compare December 20, 2016 18:48

tinayuangao added 2 commits December 21, 2016 12:09

fix lint

70650dd

Make plunker integrated

742427c

tinayuangao force-pushed the plunker branch from e723cef to 742427c Compare December 21, 2016 20:09

jelbourn merged commit 592d8ec into angular:master Dec 21, 2016


		TAGS: string[] = ['angular', 'material', 'example'];

		description: string = 'Example for Angular Material';

		@@ -0,0 +1,109 @@
		import { Http } from '@angular/http';

		export class PlunkerUtil {

feat(plnkr): Add initial version of openPlunker #26

feat(plnkr): Add initial version of openPlunker #26

Uh oh!

Conversation

tinayuangao commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Nov 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinayuangao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Dec 19, 2016

Uh oh!

jelbourn commented Dec 21, 2016

Uh oh!

jelbourn commented Dec 21, 2016

Uh oh!

tinayuangao commented Dec 21, 2016

Uh oh!

jelbourn commented Dec 21, 2016

Uh oh!

Uh oh!