Feature/code improvements n best practices #87

hardikpthv · 2017-07-01T18:37:20Z

#75 implemented

fix: Updated header format
refactor: Updated @Output label, import statements, members and methods sequence are alphabetized as per style guide.
fix: Moved service file into feature folder along with component
docs: Updated readme

…matting.

UncleDave · 2017-07-01T19:05:21Z

README.md

@@ -62,27 +62,25 @@ you can use `[headers]` directive like this.

 #### Callbacks


While we're here - though it's not strictly incorrect, I would rename this to either "Outputs" or "Events".

UncleDave · 2017-07-01T19:07:58Z

README.md

@@ -62,27 +62,25 @@ you can use `[headers]` directive like this.

 #### Callbacks

-`(onFileUploadFinish)="imageUploaded($event)"`. If `[url]` is specified this event is fired when component gets a response from the server, also in this case event has field `serverResponse` which contains the status code and response from the server `{status, response}`. If `[url]` is not specified it's fired immediately after an image(s) dropped into file-drop zone of choosed in file browser. So what you can do, is not specify `[url]` to handle upload yourself, for exapmple send the image into firebase storage. To get file use `event.file`.
+`(uploadFinish)="imageUploaded($event)"`. If `[url]` is specified this event is fired when component gets a response from the server, also in this case event has field `serverResponse` which contains the status code and response from the server `{status, response}`. If `[url]` is not specified it's fired immediately after an image(s) dropped into file-drop zone of choosed in file browser. So what you can do, is not specify `[url]` to handle upload yourself, for exapmple send the image into firebase storage. To get file use `event.file`.


By convention I think we want to go with past tense here: uploadFinished - the event has already occurred.
The example function here should also by convention be suggested as onUploadFinished($event)

All in all:

(uploadFinished)="onUploadFinished($event)"

UncleDave · 2017-07-01T19:08:39Z

README.md


-`(onRemove)="imageRemoved($event)"` - this event is fired when remove button was clicked and the image preview was removed. *Note that this library doesn't handle deletion from server so you should do it yourself*. Event passed as the argument is the exact same object that was passed to the `(imageUploaded)` callback when image was added so you can access `serverResponse` to get a key to delete your image from server.
+`(remove)="imageRemoved($event)"` - this event is fired when remove or clear button was clicked and the image preview was removed. *Note that this library doesn't handle deletion from server so you should do it yourself*. Event passed as the argument is the exact same object that was passed to the `(imageUploaded)` callback when image was added so you can access `serverResponse` to get a key to delete your image from server.


Same as above but (removed)="onRemoved($event)"

UncleDave · 2017-07-01T19:09:47Z

README.md


-`(isPending)="disableSendButton($event)"` - this event is fired when pending state was changed. Event is just a boolean that represents the pending state. Pending state is `true` when and only when component avaits a response from the server, and `false` othervise. You can use it, for example, to disable send button in your form until all images are uploaded.
+`(uploadStateChange)="disableSendButton($event)"` - this event is fired when image upload state was changed. Event is just a boolean that represents the uploading state. Image upload state is `true` when and only when component awaits the response from the server, and `false` otherwise. You can use it, for example, to disable send button in your form until all images are uploaded.


Same as above but (uploadStateChanged)="onUploadStateChanged($event)"

UncleDave · 2017-07-01T19:11:51Z

src/file-drop.directive.ts


 @Directive({
  selector: '[fileDrop]'
 })
 export class FileDropDirective {
  @Input() accept: string[];
+  @Output() isFileOver: EventEmitter<boolean> = new EventEmitter<boolean>();


Again for conventions I would go with fileOver

UncleDave · 2017-07-01T19:18:38Z

src/image-upload/image.service.ts

+
+  constructor(private http: Http) { }
+
+  public postImage(url: string, image: File, headers?: any[], partName: string = 'image',


Headers shouldn't be any.

export interface Header { [name: string]: any; }

Okay, let it be as Header itself.

UncleDave · 2017-07-01T19:20:34Z

src/image-upload/image.service.ts

+        let key = Object.keys(header)[0];
+        options.headers.append(key, header[key]);
+      }
+    }


At this point we can probably just get rid of lines 22 - 28 and replace with options.headers = headers - if that doesn't work (i'm unsure) then options.headers = new Headers(headers) should.

Yeah, this options.headers = headers is not working since Headers class has a few methods in it so continue with options.headers = new Headers() but again need to set header in expected format otherwise it goes like this 0:[object Object].

I believe the Headers constructor accepts { [name: string]: any }[]?

Okay I've been poking around in angular - it expects either a Headers object or a dictionary, which is why it wasn't happy with the array.

I suggest we just use this signature wherever we need headers, and get rid of the interface (sorry, I know I asked you to do it):

Headers|{[name: string]: any}

Then the following will work:

myFunc(headers: Headers|{[name: string]: any}) { const options: any = {}; options.headers = new Headers(headers); }

The user can pass this in by instantiating their own Headers object or something like:

const headers: {[name: string]: any} = { potato: 'cheese', cheese: 'potato' };

UncleDave · 2017-07-01T19:23:10Z

src/image-upload/image-upload.component.ts

+  @Input() buttonCaption: string = 'Select Images';
+  @Input() dropBoxMessage: string = 'Drop your images here!';
+  @Input() fileTooLargeMessage: string;
+  @Input() headers: any;


Shouldn't be any - details in image.service.ts comment.

Alright, i will set it as Header.

UncleDave · 2017-07-01T20:19:46Z

Note to merger: Squash into a conventional commit.

aberezkin

Same remarks as UncleDave's

UncleDave · 2017-07-03T08:04:42Z

Thank you for the changes so far, that all looks good - just the Headers issue to address.

UncleDave · 2017-07-04T08:50:19Z

src/image-upload/image.service.ts

+        let key = Object.keys(header)[0];
+        options.headers.append(key, header[key]);
+      }
+    }


Okay I've been poking around in angular - it expects either a Headers object or a dictionary, which is why it wasn't happy with the array.

I suggest we just use this signature wherever we need headers, and get rid of the interface (sorry, I know I asked you to do it):

Headers|{[name: string]: any}

Then the following will work:

myFunc(headers: Headers|{[name: string]: any}) { const options: any = {}; options.headers = new Headers(headers); }

The user can pass this in by instantiating their own Headers object or something like:

const headers: {[name: string]: any} = { potato: 'cheese', cheese: 'potato' };

UncleDave

Last bit of header tidy!

hardikpthv · 2017-07-05T14:21:23Z

Need to check with proposed changes. Since let options: RequestOptionsArgs = new RequestOptions(); is declared then const options: any = {}; will reset format to be sent.

hardikpthv · 2017-07-05T15:26:07Z

Changing the format from array to object { potato: 'cheese', cheese: 'potato' } solves the problem, nothing else is required. Let me finally update...

UncleDave · 2017-07-05T16:48:49Z

const options: any = {}; was just an example, I don't mean for you to do that bit 😛

hardikpthv · 2017-07-05T17:08:43Z

Alrighty then! 😆

hardikpthv · 2017-07-07T07:34:26Z

@UncleDave, demo source is updated, need to generate new bundle.

UncleDave · 2017-07-07T08:32:40Z

Okay I'm going to merge this but I still have some changes I'd like to make to headers, don't get offended 😄

hardikpthv · 2017-07-07T11:30:26Z

That's fine @UncleDave 😄

hardikpthv added 5 commits July 1, 2017 16:55

Updated @outputs label as per the style guide.

0646a6d

Updated readme file

6a5fdbc

Import, members and methods sequence alphabetized as well as code for…

38c2846

…matting.

Headers format updated and code improvements.

586cd59

Moved image service into the feature folder i.e. image-upload.

7e64893

UncleDave requested review from UncleDave and aberezkin July 1, 2017 19:02

UncleDave added this to the v1.0.0 milestone Jul 1, 2017

UncleDave requested changes Jul 1, 2017

View reviewed changes

UncleDave added task enhancement labels Jul 1, 2017

aberezkin reviewed Jul 2, 2017

View reviewed changes

hardikpthv added 2 commits July 3, 2017 11:56

Merge branch 'master' into feature/code_improvements_n_best_practices

d2280f4

fix: Naming conventions for outputs, and event handlers.

0a77883

hardikpthv added 3 commits July 4, 2017 12:42

Merge branch 'master' into feature/code_improvements_n_best_practices

fe39154

fix: Added header interface as headers type instead of any

c1eb048

fix: Code formatting

ea3d345

UncleDave requested changes Jul 4, 2017

View reviewed changes

UncleDave approved these changes Jul 4, 2017

View reviewed changes

UncleDave requested changes Jul 4, 2017

View reviewed changes

hardikpthv added 3 commits July 7, 2017 12:33

fix: Header format issue

bdb3f8e

docs: updated readme

da212d7

fix: updated demo based on repo

f052d0d

aberezkin approved these changes Jul 7, 2017

View reviewed changes

docs: updated event sequence in readme

b90e825

UncleDave merged commit 29d288e into aberezkin:development Jul 7, 2017

hardikpthv deleted the feature/code_improvements_n_best_practices branch July 7, 2017 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/code improvements n best practices #87

Feature/code improvements n best practices #87

hardikpthv commented Jul 1, 2017 •

edited

UncleDave Jul 1, 2017

UncleDave Jul 1, 2017

UncleDave Jul 1, 2017

UncleDave Jul 1, 2017

UncleDave Jul 1, 2017

UncleDave Jul 1, 2017

hardikpthv Jul 3, 2017

UncleDave Jul 1, 2017 •

edited

hardikpthv Jul 3, 2017

UncleDave Jul 3, 2017

UncleDave Jul 4, 2017

UncleDave Jul 1, 2017

hardikpthv Jul 3, 2017

UncleDave commented Jul 1, 2017

aberezkin left a comment

UncleDave commented Jul 3, 2017

UncleDave Jul 4, 2017

UncleDave left a comment

hardikpthv commented Jul 5, 2017

hardikpthv commented Jul 5, 2017

UncleDave commented Jul 5, 2017

hardikpthv commented Jul 5, 2017

hardikpthv commented Jul 7, 2017 •

edited

UncleDave commented Jul 7, 2017

hardikpthv commented Jul 7, 2017

		@@ -62,27 +62,25 @@ you can use `[headers]` directive like this.

		#### Callbacks


		`(onRemove)="imageRemoved($event)"` - this event is fired when remove button was clicked and the image preview was removed. Note that this library doesn't handle deletion from server so you should do it yourself. Event passed as the argument is the exact same object that was passed to the `(imageUploaded)` callback when image was added so you can access `serverResponse` to get a key to delete your image from server.
		`(remove)="imageRemoved($event)"` - this event is fired when remove or clear button was clicked and the image preview was removed. Note that this library doesn't handle deletion from server so you should do it yourself. Event passed as the argument is the exact same object that was passed to the `(imageUploaded)` callback when image was added so you can access `serverResponse` to get a key to delete your image from server.


		`(isPending)="disableSendButton($event)"` - this event is fired when pending state was changed. Event is just a boolean that represents the pending state. Pending state is `true` when and only when component avaits a response from the server, and `false` othervise. You can use it, for example, to disable send button in your form until all images are uploaded.
		`(uploadStateChange)="disableSendButton($event)"` - this event is fired when image upload state was changed. Event is just a boolean that represents the uploading state. Image upload state is `true` when and only when component awaits the response from the server, and `false` otherwise. You can use it, for example, to disable send button in your form until all images are uploaded.


		constructor(private http: Http) { }

		public postImage(url: string, image: File, headers?: any[], partName: string = 'image',

Feature/code improvements n best practices #87

Feature/code improvements n best practices #87

Conversation

hardikpthv commented Jul 1, 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

UncleDave Jul 1, 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

UncleDave commented Jul 1, 2017

aberezkin left a comment

Choose a reason for hiding this comment

UncleDave commented Jul 3, 2017

Choose a reason for hiding this comment

UncleDave left a comment

Choose a reason for hiding this comment

hardikpthv commented Jul 5, 2017

hardikpthv commented Jul 5, 2017

UncleDave commented Jul 5, 2017

hardikpthv commented Jul 5, 2017

hardikpthv commented Jul 7, 2017 • edited

UncleDave commented Jul 7, 2017

hardikpthv commented Jul 7, 2017

hardikpthv commented Jul 1, 2017 •

edited

UncleDave Jul 1, 2017 •

edited

hardikpthv commented Jul 7, 2017 •

edited