Convert Runner and Editor to widgets #14

kitsonk · 2017-05-23T12:52:59Z

Feature

The PR converts the Runner and Editor to widgets. It also makes a slight change to project where there is now a .getProgram() which returns a map of values which can be passed to the Runner for running the application. Previously this logic was integrated into Runner.

Resolves #6

codecov · 2017-05-23T13:00:14Z

Codecov Report

Merging #14 into master will decrease coverage by 0.38%.
The diff coverage is 95.38%.

@@            Coverage Diff            @@
##           master     #14      +/-   ##
=========================================
- Coverage   94.48%   94.1%   -0.39%     
=========================================
  Files          15      15              
  Lines         490     560      +70     
  Branches       86      96      +10     
=========================================
+ Hits          463     527      +64     
- Misses         27      33       +6

Impacted Files	Coverage Δ
src/project.ts	`87.67% <100%> (+0.71%)`	⬆️
src/support/providers/amdRequire.ts	`100% <100%> (ø)`	⬆️
src/Editor.ts	`93.87% <92.85%> (-6.13%)`	⬇️
src/Runner.ts	`97.63% <94.54%> (-2.37%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3286d9f...ad8a42f. Read the comment docs.

[ci skip]

kitsonk · 2017-05-23T13:49:02Z

This is obviously a big PR. The main focus should be on the examples/index.ts to see how a basic sort of application is built using the Runner and Editor widgets.

dylans

Minor nits... I focused mostly on typos rather that code changes.

dylans · 2017-05-24T06:34:48Z

README.md

 display files with the appropriate language intellisense as well as the file is edited, changes will automatically be sent to
 the project to keep it updated.

-The `Editor` constructor takes two arguments.  First, you need to provide a DOM element that will serve as the root for the editor.
-The second is an optional argument is a pass through of any options as specified in the `monaco.editor.IEditorOptions` interface.
+The `Editor` properties have two key properties:


"The Editor properties have two key properties:" -> "The Editor has two key properties:" ?

dylans · 2017-05-24T06:35:23Z

README.md


-The class has only one method:
+* `filename` - This is the name of the file in the project which the editor should be displaying.


"This is the name of the file in the project which the editor should be displaying." -> "Name of the file in the project which the editor should display."

Well, this one I debated, as like with widgets, you have to always set the properties. So conditional future tense is not quite appropriate, but I get the point that it could be phrased better.

dylans · 2017-05-24T06:36:07Z

README.md

 })();
 ```

 ### Runner

-This is a class which provides a simple API to run instances of a project within an `iframe` on a page.  It will automatically
-transpile the project and send the transpiled output to the `iframe`.
+This is a widget which can _run_ a program in an `iframe`.  Updating the properties related to the program on the widget will


"This is a widget which" -> "A widget which"

dylans · 2017-05-24T06:36:35Z

README.md


-The `Runner` constructor takes a single arugment, which is the `iframe` it should use to run the project in.
+The `Runner` has several key properties which control its behaviour:


behaviour -> behavior (for consistency)

dylans · 2017-05-24T06:37:05Z

README.md


-The class has one method of note:
+* `css`, `dependencies`, `html`, and `modules` - Propties that a are part of a `Program` obtained from the returned promise of


"Propties that a are part of " -> "Properties that are part of "

dylans · 2017-05-24T06:37:31Z

README.md

+  `project.getProgram()`.
+* `loader` - An optional property which specifies the URI for the AMD loader to use with the program.  It defaults to the latest
+  version of the `@dojo/loader`.
+* `src` - Is an optional URI that will be the `src` of the `iframe` until a program is first run.  This needs to be a relative URI


"Is an optional" -> "An optional"

dylans · 2017-05-24T06:38:44Z

src/Editor.ts

- * A class which is a simple abstraction of the `monaco-editor` editor
+ * @type EditorProperties
+ *
+ * Properties that can be set on Editor widget


"on Editor widget" missing word, either an or the?

dylans · 2017-05-24T06:40:00Z

src/Runner.ts

+ * @property src A URI that points to the `src` to set on the Runner's `iframe`. Defaults to
+ *               `../support/blank.html`
+ * @property onError A method that will be called whenever there is an error in the running program
+ * @property onInitIframe A method that will be called when the `iframe` is initiailized


initiailized -> initialized

dylans · 2017-05-24T06:41:35Z

src/Runner.ts

@@ -118,6 +145,50 @@ function getPackages(dependencies: { [pkg: string]: string; }): string[] {
 }

 /**
+ * Generate an HTML page which represents the Runner properties
+ * @param param0 Properties from the Runner to be used to specify the document


I'm not familiar with the param0 syntax. If that is correct, ignore me, and if not, then of course fix it.

That is what the TypeScript services stubs out. There is an open issue to actually support it, as I don't think TypeScript does at the moment.

agubler

Just a few nits and questions :)

agubler · 2017-05-24T08:10:43Z

src/Editor.ts

+export interface EditorProperties extends WidgetProperties, ThemeableProperties {
+	filename?: string;
+	options?: monaco.editor.IEditorOptions;
+


nit: why the blank line?

well, they were methods, and not necessairly in order of the previous properties. We have been putting them at the end of properties but I felt if I wan't going to order them alphabetically a new line would be a better visual indication of the "sectioning" of them.

agubler · 2017-05-24T08:11:20Z

src/Editor.ts


+/* tslint:disable:variable-name */


We have actually changed the rule for variable names for this usage in Dojo 2

I will update the tslint

agubler · 2017-05-24T08:13:44Z

src/Editor.ts

-		}));
+	public render() {
+		return v('div', {
+			afterCreate: this._initEditor,


Word of warning, afterCreate and afterUpdate are going to be removed from the VirtualDomProperties interface in a future release of widget-core.

Potentially better to use the currently exposed hooks onElementCreated and onElementUpdated in any case.

agubler · 2017-05-24T08:17:03Z

src/Editor.ts

-		if (!project.includes(filename)) {
-			throw new Error(`File "${filename}" is not part of the project.`);
+	@afterRender()
+	public updateEditor(node?: DNode): DNode | undefined {


Seems odd to use this as an afterRender and also the function to attach to the afterUpdate hook.

It was exactly the same code... I could move it to a totally private method and then call it from the two locations, but it seemed odd as well.

Hmmm yeah, why does the same code to run in an afterRender and then again on an afterUpdate?

Looking at it, I have no f*ing idea what I did that... 😢

agubler · 2017-05-24T08:18:37Z

src/Runner.ts

+export interface RunnerProperties extends Partial<Program>, WidgetProperties, ThemeableProperties {
+	loader?: string;
+	src?: string;
+


Okay same question re line break but I think I see the pattern now separating values and functions.

agubler · 2017-05-24T08:22:36Z

src/Runner.ts

+	public render() {
+		return v('div', {
+			classes: this.classes(css.base)
+		}, [ v('iframe', {


Would this be a candidate to for the element being explicitly created in the constructor (so we have a handle on the element) and then using the DomWrapper wrap it and include in the tree?

That would be a way of doing it. Are there really and advantages? The virtual DOM is just as good at creating and element as a constructor function. In addition, there are potentially other advantages with what we have been doing with subsuming DOM in the projector. The widget doesn't really care how the iframe gets there.

It means you don't need to "get" the element from widget system - which could be advantageous once those hooks have been removed.

Perhaps could then restructure the way the "afterCreate", "afterUpdate" functionality works also.

agubler · 2017-05-24T08:24:10Z

src/examples/index.ts

+			fileSelect = v('div', { key: 'fileSelect' }, [
+				v('div', [
+					v('label', { for: 'select-file' }, [ 'File to display:' ]),
+					v('select', { name: 'select-file', id: 'select-file', onchange: this._onchangeFile }, this._getFileOptions())


could use the Dojo 2 widget potentially

I thought about it. I wanted to take care of it when I did #9 as right now, it would be painful to include the CSS for the other widget. I already have to explicitly include the Runner and Editor CSS.

Issues addressed

Convert Runner and Editor to widgets

e475447

Update README with information about widgets

8189244

[ci skip]

kitsonk requested review from dylans, agubler, vansimke and bitpshr May 23, 2017 13:49

dylans previously requested changes May 24, 2017

View reviewed changes

dylans added this to the 2017.05 milestone May 24, 2017

agubler reviewed May 24, 2017

View reviewed changes

kitsonk added 3 commits May 29, 2017 11:52

Update based on feedback

3a6a3a1

Complete coverage for amdRequire

3afc45f

Updates to editor and runner styles

1d73e7d

eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017

Update package.json

ad8a42f

kitsonk merged commit 1a063c2 into dojo:master Jun 15, 2017


		The class has only one method:
		* `filename` - This is the name of the file in the project which the editor should be displaying.


		The `Runner` constructor takes a single arugment, which is the `iframe` it should use to run the project in.
		The `Runner` has several key properties which control its behaviour:


		The class has one method of note:
		* `css`, `dependencies`, `html`, and `modules` - Propties that a are part of a `Program` obtained from the returned promise of

Convert Runner and Editor to widgets #14

Convert Runner and Editor to widgets #14

Uh oh!

Conversation

kitsonk commented May 23, 2017

Uh oh!

codecov bot commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kitsonk commented May 23, 2017

Uh oh!

dylans left a comment

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!

agubler left a comment

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!

Uh oh!

codecov bot commented May 23, 2017 •

edited

Loading