Skip to content
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

fix: Make the workspace URL pretty #2101

Merged
merged 1 commit into from Jun 6, 2022
Merged

fix: Make the workspace URL pretty #2101

merged 1 commit into from Jun 6, 2022

Conversation

kylecarbs
Copy link
Member

This adds the @username/workspacename format to the
workspace page!

The only oddity I've noticed is no underline for Workspaces anymore, but I'm not sure whether it should have been anyways!
image

@kylecarbs kylecarbs self-assigned this Jun 6, 2022
@kylecarbs kylecarbs requested a review from a team as a code owner June 6, 2022 19:17
@presleyp
Copy link
Contributor

presleyp commented Jun 6, 2022

I think it's important for the user to have a cue about which page they're on (that underline we used to have). Here's the docs on it https://v5.reactrouter.com/web/api/NavLink, you probably have to update NavBarView.tsx. Oh, the problem must be that a workspace page is no longer a child of the workspaces page, right?

@kylecarbs
Copy link
Member Author

@presleyp indeed! That is the problem... I'll investigate a bit.

@presleyp
Copy link
Contributor

presleyp commented Jun 6, 2022

@kylecarbs but yeah I see your point, maybe it shouldn't say you're on the workspaces page when you're on the workspace page. Not sure what's typical there!

@kylecarbs
Copy link
Member Author

I think it makes more sense, so I'll make the change!

@presleyp
Copy link
Contributor

presleyp commented Jun 6, 2022

@f0ssel want to make sure you're happy with the solution

This adds the `@username/workspacename` format to the
workspace page!
@kylecarbs
Copy link
Member Author

Going to merge for now. @f0ssel and I chatted a while back and he seemed to be good with this route, but please comment if I'm mistaken!

@kylecarbs kylecarbs merged commit 74d9fee into main Jun 6, 2022
@kylecarbs kylecarbs deleted the wsurls branch June 6, 2022 22:53
kylecarbs added a commit that referenced this pull request Jun 7, 2022
This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.
@@ -63,7 +63,7 @@ const config: Configuration = {
port: process.env.PORT || 8080,
proxy: {
"/api": {
target: "http://localhost:3000",
target: "https://dev.coder.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

@kylecarbs Is this an expected change? Broke my local development dummy login creds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to make this a flag so this never gets in again!

@AbhineetJain
Copy link
Contributor

AbhineetJain commented Jun 7, 2022

I also found that redirection after creating a new workspace via the UI is broken. I am assuming we missed updating that URL in this PR.

@kylecarbs
Copy link
Member Author

Ahh yes. I'll fix that in my other PR for schedules, feel free to review that!

kylecarbs added a commit that referenced this pull request Jun 7, 2022
This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.
kylecarbs added a commit that referenced this pull request Jun 7, 2022
This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.
kylecarbs added a commit that referenced this pull request Jun 7, 2022
This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.
kylecarbs added a commit that referenced this pull request Jun 7, 2022
* fix: Update routing for workspace schedule

This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.

* Update site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
kylecarbs added a commit that referenced this pull request Jun 10, 2022
This adds the `@username/workspacename` format to the
workspace page!
kylecarbs added a commit that referenced this pull request Jun 10, 2022
* fix: Update routing for workspace schedule

This was broken as part of #2101. It was a silly mistake,
but unfortunate our tests didn't catch it.

This is a rare change so unlikely to occur again, so I won't
make an issue adding tests.

* Update site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
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.

None yet

4 participants