ng2-image-upload icon indicating copy to clipboard operation
ng2-image-upload copied to clipboard

Final API

Open UncleDave opened this issue 7 years ago • 11 comments

As the project has not yet hit 1.0.0 release I think we should take the opportunity to apply best practices to the API, I have the following suggestions:

Outputs should not start with on as per the angular style guide.

onFileUploadFinish changes to imageUploaded or imageUploadFinished onRemove changes to imageRemoved isPending changes to uploadStateChanged ?

I also think having our own custom Header interface is a waste of time and propose the header input property should just be a map instead:

[headers]="[{ Authorization: 'value' }]"

This seems like a more natural implementation and also directly maps to Angular's Headers class:

export declare class Headers {
    constructor(headers?: Headers | {
        [name: string]: any;
    } | null);
}

After 1.0.0 we should also endeavour to adhere to semantic versioning.

UncleDave avatar Jun 27 '17 09:06 UncleDave

Fully support this.

aberezkin avatar Jun 28 '17 05:06 aberezkin

@UncleDave, this sounds good. I can start implementing soon.

hardikpthv avatar Jun 28 '17 09:06 hardikpthv

Bear in mind to be compatible with semantic versioning these API changes would have to be v1.0.0 and not before, so it may be worth spinning up a v1.0.0 branch?

UncleDave avatar Jun 28 '17 11:06 UncleDave

Okay got it. @aberezkin We better have develop branch to make all PRs for v1.0.0. and then this can be merged to master later on.

hardikpthv avatar Jun 28 '17 14:06 hardikpthv

@hardikpthv, there is one already.

aberezkin avatar Jun 28 '17 15:06 aberezkin

The idea of the development branch is that it is persistent and contains code for the "next" version, essentially. While master is always the latest release and ideally should never be committed to except for the changelog and version number.

I believe here our development branch would be 0.6.5 -> 0.7.0 etc, while we have a parallel branch for 1.0.0 which contains things like the API change. Once 1.0.0 is ready to go it gets merged into development and the usual process continues.

UncleDave avatar Jun 28 '17 15:06 UncleDave

I can make development branch the main branch of the repo so if you mention an issue in PR it gets resolved automatically.

upd: Did it already

aberezkin avatar Jun 28 '17 16:06 aberezkin

@aberezkin, @UncleDave implemented discussed changes.

hardikpthv avatar Jul 01 '17 18:07 hardikpthv

I have a suggestion for the interface. We can use ng-content (like they do in the angular team) to provide the component with custom captions to make component usage cleaner and more intuitive.

But I didn't quite figure out how to use default messages if there was no needful content in the component's body. Also, in this case, a user can put not only string but some HTML in the message, not sure if this is even a problem.

aberezkin avatar Jul 03 '17 23:07 aberezkin

Also, I had thoughts about wrapping this lib in AbstractControl or something like that. Just thoughts, though.

aberezkin avatar Jul 04 '17 01:07 aberezkin

I think the inputs work okay for the messages, ng-content seems a bit less intuitive just because it's not used in nearly as many places as inputs. Maybe if we want to allow the user to pass in some custom HTML to display, but I don't see anyone asking for that.

AbstractControl is for validation? Unless we implement a min property I don't see us validating anything.

UncleDave avatar Jul 04 '17 08:07 UncleDave