Skip to content

Conversation

@OmerGronich
Copy link
Contributor

@OmerGronich OmerGronich commented Feb 23, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

This is a temporary design for the workspace connection screen, to allow other developers to easily connect a workspace and access the other parts of the app (generators/configuration)

@OmerGronich OmerGronich requested a review from agroupp February 23, 2023 21:33
@nx-cloud
Copy link

nx-cloud bot commented Feb 23, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6538d11. 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 targets

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #57 (6538d11) into master (5b35eeb) will increase coverage by 0.16%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   67.71%   67.88%   +0.16%     
==========================================
  Files          48       49       +1     
  Lines         254      274      +20     
  Branches       14       16       +2     
==========================================
+ Hits          172      186      +14     
- Misses         82       88       +6     
Flag Coverage Δ *Carryforward flag
cli-daemon 55.00% <ø> (ø)
cli-gui 93.93% <100.00%> (ø)
configuration 100.00% <ø> (ø) Carriedforward from 5b35eeb
executors 100.00% <ø> (ø) Carriedforward from 5b35eeb
generators 77.77% <ø> (ø)
shared ∅ <ø> (∅) Carriedforward from 5b35eeb
workspace-manager 83.78% <73.91%> (-16.22%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
apps/cli-gui/src/app/app.component.html 100.00% <ø> (ø)
...workspace-manager/workspace-manager.component.html 100.00% <ø> (ø)
...e-manager/data-access/connect-workspace.service.ts 72.72% <72.72%> (ø)
...s/connect-workspace/connect-workspace.component.ts 75.00% <72.72%> (-25.00%) ⬇️
apps/cli-gui/src/app/app.component.ts 100.00% <100.00%> (ø)
...connect-workspace/connect-workspace.component.html 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

{
provide: APP_INITIALIZER,
useFactory: (http: HttpClient, core: CoreService) => () =>
http.get<string[]>(`/api/workspace`).pipe(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need the list of projects and the current project? How do you plan to run generators?

I thought we'd keep the last workspace path in LS, so if we have one, we connect to it, and there is no need to re-connect on every page reload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right- added this logic in the guard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use sessionStorage instead, so you won't have to deal with invalidation. its also better UX IMO, the data will be persistent until the tab is closed (which should be the same as terminating the gui process)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Conflicts:
#	libs/shared/data/src/index.ts
.connectWorkspace(currentWorkspacePath)
.pipe(
map(() => {
retrySubject.next();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the implementation but we need something that can be shared and reused, maybe we should consider adding ng-query

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind, but won't it make integrating this project with the angular repo more difficult? It might be a good idea to verify with them before relying on 3rd party libraries that aren't 100% necessary.

{
provide: APP_INITIALIZER,
useFactory: (http: HttpClient, core: CoreService) => () =>
http.get<string[]>(`/api/workspace`).pipe(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use sessionStorage instead, so you won't have to deal with invalidation. its also better UX IMO, the data will be persistent until the tab is closed (which should be the same as terminating the gui process)

@OmerGronich OmerGronich requested review from OmerGBlink, agroupp, ronnetzer and tomzohar and removed request for OmerGBlink February 26, 2023 19:41
@OmerGronich OmerGronich merged commit c89acbd into e-square-io:master Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants