feat: implement tech extension upload and most of the detail page #412
Conversation
gashcrumb
added
group/ui
| // WIP | ||
| this.subscription = this.store.resource.subscribe(extension => { | ||
| this.extension = extension; | ||
| }); |
There was a problem hiding this comment.
You can streamline the component implementation by using observables as is, so you can declutter the component by removing subscriptions and stuff. First, turn the extensionclass field into this:
extension$: Observable<Extension>;
And then remove the subscription above and use this expression instead:
this.extension$ = this.store.resource;
TIP: If you name object instances by the camelCased version of its injected type - ExtensionStore in this case, code becomes more expressive. The above would look more self-explanatory if it was
this.extension$ = this.extensionStore.resource;
Now you can completely remove the subscription: Subscription field and the ngOnDestroy blocks below. The only action required then will be to go the components template and apply this simple change:
<syndesis-loading [loading]="loading | async">
<div class="row" *ngIf="extension$ | async; let extension = extension$">And that's it! ;)
There was a problem hiding this comment.
FYI, the Angular engine automatically "completes" any observable binding piped with the async pipe. That is why it is recommended to bind observable information directly, rather than subscribing to it (which entails tearing down the subscriptions by hand upon disposing the component and stuff)
|
|
||
| uploader: FileUploader; | ||
| response: Extension = undefined; | ||
| error: FileError = undefined; |
There was a problem hiding this comment.
Why are these fields initialized as undefined? I've seen this pattern repeated all over the app and I still cannot wrap my head around on why we are doing this.
There was a problem hiding this comment.
Habit, will take that out :-)
| @@ -10,6 +11,7 @@ | |||
| } | |||
| } | |||
| } | |||
| */ | |||
There was a problem hiding this comment.
If this piece of CSS is no longer required, I'd say we should remove it instead of commenting it out.
There was a problem hiding this comment.
Yup, totally meant to do that, will take it out.
| @@ -1,6 +1,7 @@ | |||
| @import 'patternfly-sass/assets/stylesheets/patternfly/color-variables'; | |||
| .integrations.list { | |||
| color: #656565; | |||
There was a problem hiding this comment.
Harcoded hex colors in component stylesheets make application theming more complex upon time. Don't we have a centralized _palette.scss partial or something? I'd move this one there.
There was a problem hiding this comment.
We do on the line above. I don't even recall what this is for and it doesn't match any patternfly colors so I'll take it out.
| return this.restangularService.one().getRestangularUrl(); | ||
| } | ||
|
|
||
| public importExtension(id: string) { |
There was a problem hiding this comment.
I miss the type annotation in these methods, so one can spot at a glance what do they return instead of having to inspect them through the IDE's tschecker or looking into the method body.
|
|
||
| public importExtension(id: string) { | ||
| return this.service.importExtension(id); | ||
| } |
There was a problem hiding this comment.
I'd annotate the methods with their returning types.
|
|
||
| @Component({ | ||
| selector: 'syndesis-tech-extensions-list', | ||
| templateUrl: 'tech-extensions-list.component.html' | ||
| templateUrl: 'tech-extensions-list.component.html', | ||
| styleUrls: ['./tech-extensions-list.component.scss'] | ||
| }) | ||
| export class TechExtensionsListComponent implements OnInit { | ||
| extensions: Observable<Extensions>; |
There was a problem hiding this comment.
The convention for observable fields and variables is to name them with a trailing $ (extensions$ in this case), so one can recognize at a glance that this variable or that class property is observable and hence requires to be handled with special care.
| level: 'alert alert-danger', | ||
| message: resp.userMsg || 'An unknown error has occurred' | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I'd say this is too much logic for a constructor. I know that constructors are meant to initialize stuff but perhaps this is too much. On the other hand most of this logic seems that might be of interest for other components that require upload functionalities.
Perhaps it would be worth to promote this to a parent class this component might extend from or even an injectable service that receives the uploader, error, response and fileSelect in its initialize payload.
There was a problem hiding this comment.
All this code is basically for initializing the file upload widget, first creating an instance and then overriding some handlers. I could move it to ngOnInit I guess.
|
@deeleman does this look ok to you ? If so, could you please approve to the this PR merged thanks ;-) ... |
Makes it possible to upload a tech extension via the UI and import it, fixes the list so it works properly, also adds some of the detail page. Need to implement deleting and updating tech extensions, as well as fetch them via the step store (or somehow) in the integration editor.
Relates to #262 #263
Fixes #264
@lburgazzoli FYI...