-
Notifications
You must be signed in to change notification settings - Fork 937
fix: create and read workspace page #1294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1294 +/- ##
==========================================
+ Coverage 66.08% 66.43% +0.34%
==========================================
Files 281 282 +1
Lines 18424 18466 +42
Branches 220 231 +11
==========================================
+ Hits 12176 12268 +92
+ Misses 4984 4940 -44
+ Partials 1264 1258 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -15,13 +15,19 @@ export interface CreateWorkspaceForm { | |||
template: Template | |||
onSubmit: (request: CreateWorkspaceRequest) => Promise<Workspace> | |||
onCancel: () => void | |||
organization_id: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be organizationID
to match other camelCase
fields in this type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it snakecased because it gets piped directly out of and into api calls, but I agree the inconsistency isn't good. I guess I'll ask about this at FE V, we have some other spots like this.
const workspaceId = firstOrItem(workspaceQueryParam, null) | ||
|
||
const xServices = useContext(XServiceContext) | ||
const [workspaceState, workspaceSend] = useActor(xServices.workspaceXService) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this uses a global XState service. Would useMachine
be more appropriate here since the machine should only be active for the lifecycle of this page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it would be active for longer but maybe that's not actually desired since it's for a specific workspace. I can make it local for now and useMachine
and we can hoist it later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neato neato
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylecarbs Actually I think we'll want it to outlive the component because we're using separate pages for things like the create/edit form and the parts of the timeline. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
I wonder if we could tweak the workspace endpoint in the future to include more info so we can avoid three calls on the FE but I might be overthinking it 😂
* Change name of existing workspace call * Add new api call (has handler already) * WorkspacesPage -> WorkspacePage * starting to replace swr * Add other api calls * Fix api call * Replace swr with xstate * Format * Test - wip * Fix route in template page * Fix endpoint in create workspace * Fix tests * Lint
This lays groundwork for #1032
renderWithAuth
to work with parameterized routes