[WIP] feat: add dialog to preview output files (images only) #536

d3lm · 2017-09-29T16:24:47Z

No description provided.

cburgdorf

Reviewed! Great work. Left some comments.

cburgdorf · 2017-10-02T10:05:59Z

client/src/app/util/location-helper.ts

@@ -3,6 +3,8 @@ import { NavigationExtras, Router, UrlSerializer } from '@angular/router';
 import { Location, LocationStrategy  } from '@angular/common';
 import { WindowRef } from '../window-ref.service';

+const omit = require('lodash.omit');


TBH, I'm a bit sceptical if we really need to pull in yet another library for this. Especially since I'm not completely sure about the footprint. The whole omit npm package is 1.5k lines of code https://github.com/lodash/lodash/blob/4.5.0-npm-packages/lodash.omit/index.js

Considering that what we really need is basically just to remove the preview key, I'm unsure if this is really justified.

It also seems depricated lodash/lodash#2930 (comment)

cburgdorf · 2017-10-02T10:08:22Z

client/src/app/output-files.service.ts

@@ -41,5 +41,9 @@ export class OutputFilesService {
        .startWith(false)
        .take(2)
  }
+
+  isImage(fileName: string) {


Can we put that somewhere into util? Basically just

export const isImage = fileName => (/\.(gif|jpg|jpeg|tiff|png)$/i).test(fileName)

cburgdorf · 2017-10-02T10:14:26Z

client/src/app/util/location-helper.ts

+    this.location.go(this.urlSerializer.serialize(currentUrlTree));
+  }
+
+  removeQueryParams(path: string, keys: Array<string> | string) {


Seem my comment above. I realize you want a generic method and omit seems handy but for the reasons stated above I'd vote to either:

Make this less generic and just go with delete preview as this is really all that is currently needed

Implement this removeQueryParams method without pulling in lodash omit

Great point! I'll change it. My idea was to make it very generic to that you can specify a range of query params to be deleted. But for now it works. Even if we later need this we can also go with a simple for loop or something and use delete.

cburgdorf · 2017-10-02T10:18:46Z

client/src/app/lab-editor/file-preview/file-preview-dialog-ref.ts

+
+import 'rxjs/add/operator/first';
+
+export class FilePreviewDialogRef<T> {


You are introducing generic T but you don't seem to use it in any of the APIs. Instead all these APIs are using any. Didn't you mean to use T there?

cburgdorf · 2017-10-02T10:20:50Z

client/src/app/lab-editor/file-preview/file-preview-dialog-ref.ts

+  close(dialogResult?: any): void {
+    this.containerInstance.animationStateChanged
+      .filter(event => event.phaseName === 'start')
+      .first()


I think this should rather be take(1)

cburgdorf · 2017-10-02T10:21:03Z

client/src/app/lab-editor/file-preview/file-preview-dialog-ref.ts

+
+    this.containerInstance.animationStateChanged
+      .filter(event => event.phaseName === 'done' && event.toState === 'leave')
+      .first()


I think this should rather be take(1)

0x-r4bbit

Looks good!

Left some minor comments.

0x-r4bbit · 2017-10-02T10:23:07Z

client/src/app/lab-editor/file-outputs/file-outputs.component.ts

@@ -1,17 +1,19 @@
-import { Component, OnInit, OnDestroy, OnChanges, Input } from '@angular/core';
+import { Component, OnInit, OnDestroy, OnChanges, Input, ViewContainerRef, ComponentRef } from '@angular/core';


Both ViewContainerRef and ComponentRef are imported but not used.

0x-r4bbit · 2017-10-02T10:23:30Z

client/package.json

@@ -14,19 +14,20 @@
  "private": true,
  "dependencies": {
    "@angular/animations": "5.0.0-beta.6",
-    "@angular/cdk": "2.0.0-beta.10",
+    "@angular/cdk": "^2.0.0-beta.11",


Can we either

rollback to 2.0.0-beta.10 here or

make a separate commit that upgrades dependencies?

In fact I'd prefer to have rollback and have a single commit later for upgrading Angular Material, as it introduces a very big deprecation that I'd like to get out of our way asap: https://github.com/angular/material2/blob/master/CHANGELOG.md#200-beta11-carapace-parapet-2017-09-21

I need the update in order to import PortalInjector.

0x-r4bbit · 2017-10-02T11:42:16Z

client/src/app/lab-editor/file-outputs/file-outputs.component.ts

 import { Observable } from 'rxjs/Observable';
 import { OutputFilesService } from '../../output-files.service';
 import { OutputFile } from '../../models/output-file';
-
-import 'rxjs/add/operator/scan';


Any reason this has been removed? The scan operator is still being used in this file.

Cause it was already inside the rx.operators.ts. Do we gather the operators there or keep them in the components? Cause I see those operator imports sprinkled all over the app.

See #536 (comment)

Oh I see. Makes sense.

0x-r4bbit · 2017-10-02T11:43:14Z

client/src/app/lab-editor/file-preview/file-preview-dialog-ref.ts

+import { Subject } from 'rxjs/Subject';
+import { FilePreviewDialogComponent } from './file-preview-dialog/file-preview-dialog.component';
+
+import 'rxjs/add/operator/first';


Need filter as well

We started to have operators in a separate file rx.operators.ts. This one just needs to be removed here.

We actually moved away from using rx.operators.ts a while ago and rather import every used operator in their dedicated files. This is because e.g. we can't lazyload operators if we put them all in one file.

The only case where we still use rx.operators.ts is in tests.

0x-r4bbit · 2017-10-02T11:56:20Z

client/src/app/lab-editor/file-preview/file-preview-dialog/file-preview-dialog.component.ts

+
+  private onImageLoaded() {
+    this.loading = false;
+    this.imageElement.nativeElement.src = this.outputFile.download_url;


Can we use Renderer Apis here instead?

Yep I can do that.

Wait, why Renderer? I am using a ViewChild here. Do I really need the Renderer APIs?

0x-r4bbit · 2017-10-02T11:58:02Z

client/src/app/lab-editor/file-preview/file-preview.tokens.ts

+export const FILE_PREVIEW_DIALOG_SCROLL_STRATEGY_PROVIDER = {
+  provide: FILE_PREVIEW_DIALOG_SCROLL_STRATEGY,
+  deps: [Overlay],
+  useFactory: FILE_PREVIEW_DIALOG_SCROLL_STRATEGY_PROVIDER_FACTORY,


Trailing comma

0x-r4bbit · 2017-10-02T12:01:16Z

client/src/app/lab-editor/file-preview/file-preview.tokens.ts

+export const FILE_PREVIEW_DIALOG_SCROLL_STRATEGY =
+  new InjectionToken<() => ScrollStrategy>('mat-dialog-scroll-strategy');
+
+export function FILE_PREVIEW_DIALOG_SCROLL_STRATEGY_PROVIDER_FACTORY(overlay: Overlay):


Any reason this factory function is written in uppercase letters everywhere?

Just nitpicking here, but I prefer having functions be camel cased (I know Angular core does the same in some places but I don't think we need to follow that convention)

Take a look at app.module.ts where we have a factory provider for the firebase database, as an example

Agreed. Looks weird. Missed that.

0x-r4bbit · 2017-10-02T12:02:20Z

client/src/app/util/location-helper.ts

+    this.location.go(this.urlSerializer.serialize(currentUrlTree));
+  }
+
+  removeQueryParams(path: string, keys: Array<string> | string) {


Can we have this method addition in a separate commit (including tests)?

Why in a separate commit? I mean it's part of this feature.

I'd prefer having a separate commit for that and another one that introduces the file preview so that if you ever have to cherry-pick/rollback you don't have get back everything or nothing, but rather can pick the specific parts you need.

I agree it prolly won't be the case in this scenario, but you never know. Better safe than sorry.

cburgdorf

LGTM

Left another comment regarding your change to remove generics completely. Not a show stopper just want to know if there was a specific reason why you went with a non generic / any version rather than a type safe one?

cburgdorf · 2017-10-02T16:31:38Z

client/src/app/lab-editor/file-preview/file-preview-dialog-ref.ts

+import 'rxjs/add/operator/filter';
+import 'rxjs/add/operator/take';
+
+export class FilePreviewDialogRef {


I see that you removed the type param here. Why did you not keep it and use it in the methods? All you methods are using any. Was there anything preventing you from using T in those methods?

Cause the generic was not needed as the component for the overlay will always be FilePreviewDialogComponent. The flexibility from the MdDialog service was not needed for this specific use case (at least not yet). At this point we don't need the ability to pass a component to the service that will be rendered inside a PortalHost. Once we need this we can easily refactor it back to a more generic approach. Does that make sense?

Plus I am only using any for the Subjects and the Observable that emits the result passed to close(). This of course can be of any type. The generic, again, is only needed if you want to have multiple types of components that can be rendered.

I thought the generic was meant to be for the type of the dialog result, no?

I see two options here:

The dialog result always has the same shape -> create an interface and use that instead of any

The dialog result can be set dynamically -> make this class generic and use T in those methods.

There shouldn't be a reason to define these APIs without type safety.

OK makes sense. Then I re-introduce the generic and use that for the result. Thanks for the valuable input / feedback.

The question is, do we even need a result? I mean what do we expect from such overlay? Right now I can't think of anything that may be passed as a result from the FilePreviewDialogComponent.

I thought the generic was meant to be for the type of the dialog result, no?

Not really. I have introduced this because I thought I make it very generic similar to the MdDialog. Then I found that I don't need this and the generic type <T> became a left over. There's only one component instantiated which is the FilePreviewDialogComponent. There's no dynamic component as with the MdDialog.

Yep, kill it with fire 💣

I now decided to use OutputFile as the type for the result. In Google Drive you can edit the file in the overlay which I find pretty cool and probably useful. When we add such functionality we can easily return the updated outputFile and then pass it along to a service that updates the file in the DB using Firebase. What do you think @cburgdorf @PascalPrecht?

Alternatively I can get rid of the result completely as we don't need it so far.

Just get rid off it for now

d3lm · 2017-10-05T10:33:02Z

client/src/app/lab-editor/file-preview/testing/dispatch-events.ts

@@ -0,0 +1,37 @@
+// Taken from https://github.com/angular/material2/blob/master/src/cdk/testing


This whole file can probably be extracted into a general testing util. These functions are really helpful when dispatching keyboard events.

I'd rather not have source code copied over and separately version controlled in our own projects, but if we really needed then yes, this should be moved to src/test-helpers

cburgdorf · 2017-10-06T10:04:26Z

Anything left to do here? I didn't follow the discussion around Angular Material. So, from my side there's nothing blocking this from getting merged any more.

0x-r4bbit

Cool @d3lm ! We're getting there.

Left a few more comments. I think once we get those ironed out this can be merged.

0x-r4bbit · 2017-10-06T10:17:48Z

client/src/app/lab-editor/file-preview/file-preview-dialog.service.spec.ts

+          }
+        }
+      ]
+    }).compileComponents();


We don't need .compileComponents() here because angular CLI takes care of compiling all the templates and styles into the components.

0x-r4bbit · 2017-10-06T10:18:49Z

client/src/app/lab-editor/file-preview/file-preview-dialog.service.spec.ts

+
+  beforeEach(() => {
+    viewContainerFixture = TestBed.createComponent(TestComponent);
+    viewContainerFixture.detectChanges();


Just a nitpick here, but this could as well just be in the beforeEach() before this one. There's no need to introduce a second beforeEach() block.

0x-r4bbit · 2017-10-06T10:19:12Z

client/src/app/lab-editor/file-preview/file-preview-dialog.service.spec.ts

+      size_bytes: 204607,
+      user_id: '5Yh73eYomSfTOcHFUx5EzkN5r1B3'
+    }
+  })


Same here, this can all be done in the first beforeEach block

0x-r4bbit · 2017-10-06T10:20:53Z

client/src/app/lab-editor/file-preview/file-preview-dialog.service.spec.ts

+    viewContainerFixture.detectChanges();
+
+    expect(overlayContainerElement.querySelector('.cdk-overlay-backdrop').classList).toContain('dark-backdrop');
+    expect(overlayContainerElement.querySelector('.cdk-overlay-pane').classList).toContain('ml-file-preview-dialog-panel');


How does this test any config defaults? I see you're configuring the output file for the data configuration but you're testing class names.

I'm sure I'm missing something.

Nevermind, seeing it in the next spec

0x-r4bbit · 2017-10-06T10:23:42Z

client/src/app/lab-editor/file-preview/file-preview-dialog.service.spec.ts

+      expect(backdropClickSpy).toHaveBeenCalledTimes(1);
+      done();
+    });
+  });


This looks to me like you're testing if Angular's emitter's do their job.

Is this really needed?

That's right but I thought it would be good to also test this, if the streams emit when the dialog is closed. I am testing the whole process of closing the dialog and when you execute close it doesn't necessarily mean that the streams emit values because I have certain filters on the pipeline. So it also tests if the animationState changes and only then the stream(s) should emit a value.

0x-r4bbit · 2017-10-06T10:32:30Z

client/src/app/lab-editor/file-preview/file-preview-dialog/file-preview-dialog.component.ts

@@ -0,0 +1,65 @@
+import { Component, Inject, EventEmitter } from '@angular/core';
+import { MD_DIALOG_DATA } from '@angular/material';


This is not used anywhere

0x-r4bbit · 2017-10-06T10:34:04Z

client/src/app/lab-editor/file-preview/file-preview-dialog/file-preview-dialog.component.ts

+  loading = true;
+  error = null;
+
+  animationState: 'void' | 'enter' | 'leave' = 'enter';


Just thinking out loud here: Would it make sense to make those string enums?

It's only used inside the class and an enum generates more code. Using string literal types gives you type safety and less code generated (as it generates no additional code).

0x-r4bbit · 2017-10-06T10:36:14Z

client/src/app/lab-editor/file-preview/file-preview-dialog/file-preview-dialog.component.ts

+
+  onError(error: ErrorEvent) {
+    this.loading = false;
+    this.error = 'Bummer! An error occured while loading the image.';


This message should rather be moved into the template.

0x-r4bbit · 2017-10-06T10:38:15Z

client/src/app/lab-editor/file-preview/testing/dispatch-events.ts

@@ -0,0 +1,37 @@
+// Taken from https://github.com/angular/material2/blob/master/src/cdk/testing


I'd rather not have source code copied over and separately version controlled in our own projects, but if we really needed then yes, this should be moved to src/test-helpers

0x-r4bbit · 2017-10-06T10:39:02Z

client/src/app/shared/dialog/dialog-error.component.scss

+:host {
+  display: flex;
+  align-items: center;
+}


This little bit of CSS can be inlined on the component. No need to have a separate file for that.

cburgdorf reviewed Oct 2, 2017

View reviewed changes

0x-r4bbit requested changes Oct 2, 2017

View reviewed changes

cburgdorf reviewed Oct 2, 2017

View reviewed changes

machinelabs deleted a comment from cburgdorf Oct 3, 2017

d3lm commented Oct 5, 2017

View reviewed changes

0x-r4bbit requested changes Oct 6, 2017

View reviewed changes

d3lm added 3 commits October 6, 2017 12:42

chore: update Angular Material and CDK to 2.0.0-beta.11

b0ec54a

feat: add removeQueryParams to LocationHelper

46fc726

feat: add dialog to preview output files (images only)

8578bd0

0x-r4bbit merged commit b0b4fb3 into machinelabs:master Oct 6, 2017

0x-r4bbit deleted the dominic/feat/output-preview branch October 6, 2017 11:47


		import 'rxjs/add/operator/first';

		export class FilePreviewDialogRef<T> {

		@@ -1,17 +1,19 @@
		import { Component, OnInit, OnDestroy, OnChanges, Input } from '@angular/core';
		import { Component, OnInit, OnDestroy, OnChanges, Input, ViewContainerRef, ComponentRef } from '@angular/core';

		@@ -0,0 +1,37 @@
		// Taken from https://github.com/angular/material2/blob/master/src/cdk/testing

		@@ -0,0 +1,65 @@
		import { Component, Inject, EventEmitter } from '@angular/core';
		import { MD_DIALOG_DATA } from '@angular/material';

[WIP] feat: add dialog to preview output files (images only) #536

[WIP] feat: add dialog to preview output files (images only) #536

Conversation

d3lm commented Sep 29, 2017

cburgdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3lm Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3lm Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3lm Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3lm Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Oct 6, 2017

0x-r4bbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3lm Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit Oct 2, 2017 •

edited

d3lm Oct 2, 2017 •

edited

d3lm Oct 2, 2017 •

edited

d3lm Oct 2, 2017 •

edited

d3lm Oct 3, 2017 •

edited

d3lm Oct 6, 2017 •

edited