-
Notifications
You must be signed in to change notification settings - Fork 27
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
chore(workspaces): create button and test #3586
Conversation
🦋 Changeset detectedLatest commit: 4e93bf8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
92ca9bd
to
1347006
Compare
...lication-shell/src/components/workspaces-navigation-button/workspaces-navigvation-button.tsx
Outdated
Show resolved
Hide resolved
|
||
export const FLAGS = {}; | ||
|
||
// Long-lived feature flags, defined in the MC API. | ||
export const DEFAULT_FLAGS = { | ||
[CUSTOM_VIEWS]: { value: true }, | ||
[ENABLE_WORKSPACES_UI]: { value: false }, |
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.
Reviewers - to see this in action locally, toggle to true
/** | ||
* If we don't have a project key, there's no way to navigate to the workspaces page in the fallback application. | ||
* Once Workspaces lives in a centralized location, we can navigate to that specific URL. | ||
* | ||
* TODO: Reevaluate this after the iterative launcher / junior is released. | ||
*/ | ||
if (!projectKey) return null; |
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'm confused. Isn't the workspaces URI path /workspaces
? There is no need for a project key...
cc @ByronDWall
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 too am confused - I assumed we needed the project key for Junior since it's hosted in the fallback application (for now)
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.
There is some ongoing work to update the proxy to handle /workspaces
as a requested path. From the RFC:
workspaces should be added to the staticUriPathsInPositionOfProjectKey array in the get-proxy-target-for-application util. Tests for proxy behavior when request path only contains the first position "projectKey" should be updated to test for correct behavior when the requested path is workspaces.
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.
The routing can handle either /:projectKey/workspaces
or /workspaces
- will adjust accordingly
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.
The client-side router in application-fallback
can handle either /:projectKey/workspaces
or /workspaces
.
The URI path is /workspaces
without the project key.
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.
@ByronDWall @emmenko See c2b0dc9 for a simplified approach
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.
Beauty
Summary
This PR creates a new button in the AppBar component that will navigate to the Workspaces page.
Additional comments inline.