feat: connect workspace filesystem navigator#106
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1af8843. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 64.34% 66.40% +2.06%
==========================================
Files 77 81 +4
Lines 589 643 +54
Branches 27 32 +5
==========================================
+ Hits 379 427 +48
- Misses 209 214 +5
- Partials 1 2 +1
*This pull request uses carry forward flags. Click here to find out more.
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| const path$ = currentWorkspacePath | ||
| ? of(currentWorkspacePath) | ||
| : this.workspaceManagerApiService.getHomeDir(); | ||
| path$.subscribe((path: string) => this.path$.next(path)); |
There was a problem hiding this comment.
Don't you need to handle unsubscribing?
There was a problem hiding this comment.
the of observable completes after emitting and the http observable also completes
| this.initWorkspacePath(); | ||
| }); | ||
|
|
||
| combineLatest([this.path$, this.pathSeparator$]) |
There was a problem hiding this comment.
directories$ could just be an observable and you can remove the takeUntil
| combineLatest([this.path$, this.pathSeparator$]) | |
| this.directories$ = combineLatest([this.path$, this.pathSeparator$]).pipe( | |
| switchMap(([path, separator]) => { | |
| path && separator && this.setIsAngularWorkspace(path, separator); | |
| return path | |
| ? this.workspaceManagerApiService.getDirectoriesInPath(path) | |
| : of([]); | |
| }), | |
| ) |
| this.workspaceManagerApiService | ||
| .getPathSeparator() | ||
| .subscribe((separator) => { | ||
| this.pathSeparator$.next(separator); |
There was a problem hiding this comment.
pathSeparator$ can also be an observable, the same as directories$
| <button | ||
| mat-raised-button | ||
| color="primary" | ||
| [disabled]="(isAngularWorkspace$ | async) === false" |
There was a problem hiding this comment.
IMO it should be hidden entirely if not angular workspace (having a disabled button without some explanation can be confusing)
There was a problem hiding this comment.
I guess the explanation is in the icons of the file system
if you click an angular workspace folder the connect button is enabled
if you click a regular folder it's disabled
| { | ||
| provide: WorkspaceManagerApiService, | ||
| useValue: workspaceManagerApiServiceMock, | ||
| }, |
There was a problem hiding this comment.
instead of exposing getWorkspaceManagerApiServiceMock(), expose a provideWorkspaceManagerTesting() that returns an Provider[] and includes all the mocks that are needed for testing workspace-manager (including connect-workspace)
| path$.subscribe((path: string) => this.path$.next(path)); | ||
| } | ||
|
|
||
| private setIsAngularWorkspace(path: string, separator: string): void { |
There was a problem hiding this comment.
IMO it should be part of filesystem-navigator, and you should add an output to that component instead of isAngularWorkspace$ behavior subject you have here
There was a problem hiding this comment.
so this is what i did in the last PR and you and @OmerGronich said to keep the component dumb
I also think that this calculation should be done inside the navigator
| private destroyed$ = new Subject<boolean>(); | ||
|
|
||
| ngOnInit(): void { | ||
| this.workspaceManagerApiService |
There was a problem hiding this comment.
there are too many subscribes that ends up assigning the returned value into another subject that is being subscribed in the template with async pipe. try to think how this can be avoided and instead of subjects and subscribes in ts work as much as you can with streams that are being subscribed with async pipe
| <ng-container | ||
| actions | ||
| [ngTemplateOutlet]="fsNavigatorActions" | ||
| ></ng-container> |
There was a problem hiding this comment.
I think I would've put it directly in the toolbar component instead of using ng-content twice (this goes with the previous comment of moving the isAngularWorkspace logic into the navigator component)
4 participants
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
implement filesystem navigator in connect workspace domain
What is the current behavior?
using a form input to get the absolute path to the workspace
Issue Number: #7
What is the new behavior?
allowing the user to navigate his filesystem and connect any angular workspace
Does this PR introduce a breaking change?
Other information