-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Replace /workspaces → /projects as default landing page for both users and teams #6048
Conversation
/approve |
Looking at this ...when the build is finished! 👀 |
Thanks @gtsiolis! I feel like the Projects page is missing some Currently, with this PR, the "shortest path to a workspace" is:
|
See also: #6047 (comment) (possibly related to this change) |
b08beed
to
c653f05
Compare
Added a drive-by change: A EDIT: Spliced this commit out into its own PR: #6185 |
36ff5e3
to
f74bb72
Compare
f74bb72
to
55c8f6b
Compare
Still waiting for a review! 😊 🙏 /werft run 👍 started the job as gitpod-build-jx-projects-default-page.5 |
Thanks for this PR! |
LGTM label has been added. Git tree hash: d3412d2917956887e600fa9e04d78dfb26f6a610
|
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 had a draft review that I never submitted here. 😭
Thanks for the reminder, @jankeromnes! ⏰
thought(non-blocking): Overall, looks great and into the right direction. However, posting here for visibility that this change would change the existing onboarding flow and could slightly affect usage.
Approving to unblock merging but holding while @laushinka also looks at this[1]. 👀
/hold
@@ -395,7 +395,7 @@ export async function deployToDev(deploymentConfig: DeploymentConfig, workspaceF | |||
werft.fail('deploy', err); | |||
} finally { | |||
// produce the result independently of Helm succeding, so that in case Helm fails we still have the URL. | |||
exec(`werft log result -d "dev installation" -c github-check-preview-env url ${url}/workspaces/`); | |||
exec(`werft log result -d "dev installation" -c github-check-preview-env url ${url}/projects`); |
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.
praise: Nice one! 💯
@@ -395,7 +395,7 @@ export async function deployToDev(deploymentConfig: DeploymentConfig, workspaceF | |||
werft.fail('deploy', err); | |||
} finally { | |||
// produce the result independently of Helm succeding, so that in case Helm fails we still have the URL. | |||
exec(`werft log result -d "dev installation" -c github-check-preview-env url ${url}/workspaces/`); | |||
exec(`werft log result -d "dev installation" -c github-check-preview-env url ${url}/projects`); |
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.
question: Probably going with /
this could also work, right?
customFontStyle: 'text-red-600 dark:text-red-400 hover:text-red-800 dark:hover:text-red-300', | ||
onClick: () => onRemoveProject(p) | ||
}]} /> | ||
<ContextMenu menuEntries={[ |
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.
issue: This is out of the scope of these changes but do you think it'd be easy to add a hover state on the more action button as seen everywhere else?
Adding hover:bg-gray-200 dark:hover:bg-gray-700 rounded-md
in the Context Menu component could suffice unless I'm missing something. What do you think? Feel free to open a follow up PR if needed.
const children = props.children || <svg className="w-8 h-8 p-1 text-gray-600 dark:text-gray-300" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><title>Actions</title><g fill="currentColor" transform="rotate(90 12 12)"><circle cx="1" cy="1" r="2" transform="translate(5 11)" /><circle cx="1" cy="1" r="2" transform="translate(11 11)" /><circle cx="1" cy="1" r="2" transform="translate(17 11)" /></g></svg>; |
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.
Good catch! Pushed a new drive-by commit to fix this. 🚀
EDIT: In #6185, that is.
}]} /> | ||
<ContextMenu menuEntries={[ | ||
{ | ||
title: "New Workspace", |
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.
praise: YES!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gtsiolis, jankeromnes, laushinka Associated issue: #5050 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
If I've added an lgtm, do I also have to add an approve tag? |
@laushinka no need 🙂 you can see in @roboquat's comment above that your "lgtm" also counted as an "approve":
If you're wondering why this PR isn't merged, that's because of the "do-not-merge/hold" label. Before we remove that and merge this PR, I think we need more buy-in for the new default location, especially from product. Relatedly, what do you think of this @jldec? |
If this will make the "new project" onboarding flow the default first experience for new users, I would recommend holding this for now. I think the new project flow is still a bit long and can be difficult to understand in places, especially for noobs like myself. See screenshots (internal only) |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I think we should move forward with this. /unhold |
Description
Related Issue(s)
Fixes #5050
How to test
Release Notes