Skip to content

Open start workspace in modal from projects list #15914

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 23, 2023
Merged

Open start workspace in modal from projects list #15914

merged 3 commits into from
Jan 23, 2023

Conversation

selfcontained
Copy link
Contributor

@selfcontained selfcontained commented Jan 20, 2023 •
edited
Loading

Description

This adjusts the Start Workspace UI from the Projects list page and Branches page to open in a modal vs. redirecting to a new page. This allows the modal to be closed, and avoids losing your context of what you were doing when you opened it.

While in here, I also noticed the RepositoryFinder component was continuously making calls to getSuggestedContextURLs due to a useEffect() dep that it mutate, so I corrected that was well.

I added an option to the StartWorkspaceModal to allow the contextUrl to be changed even if passed in, as locking it felt unnecessary here, although we do default it to the project's url.

image

Related Issue(s)

Fixes #15696

How to test

  • On the preview environment, create a new team and project
  • On the Projects list page, click the overflow menu, and select New Workspace...
  • It should open the Start Workspace modal instead of navigating to a new page
  • It should default the context url based on the project you selected
  • You should still be able to change the context url even though it defaulted

Release Notes

NONE

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-bmh-allow-users-to-close-15696.2 because the annotations in the pull request description changed
(with .werft/ from main)

@selfcontained
Copy link
Contributor Author

selfcontained commented Jan 20, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-bmh-allow-users-to-close-15696.3
(with .werft/ from main)

selfcontained
selfcontained commented Jan 20, 2023
View reviewed changes
components/dashboard/src/components/RepositoryFinder.tsx
@@ -31,7 +31,7 @@ export default function RepositoryFinder(props: RepositoryFinderProps) {
setSuggestedContextURLs(urls);
saveSearchData(urls);
});
}, [suggestedContextURLs]);
}, []);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this effect updates suggestedContextURLs, having it as a dep caused it to constantly run as soon as it calls setSuggestedContextURLs

selfcontained
selfcontained commented Jan 20, 2023
View reviewed changes
components/dashboard/src/workspaces/StartWorkspaceModal.tsx
@@ -17,6 +17,8 @@ export interface StartWorkspaceModalProps {
ide?: string;
workspaceClass?: string;
contextUrl?: string;
// If contextUrl is provided, setting `allowContextUrlChange` to true will allow it to be changed still
allowContextUrlChange?: boolean;
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 didn't want to change existing behavior for when the start workspace ui locks that context url, so adding this option felt like the safest approach.

@selfcontained selfcontained marked this pull request as ready for review January 20, 2023 16:06
@selfcontained selfcontained requested a review from a team January 20, 2023 16:06
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jan 20, 2023
@jldec
Copy link
Contributor

jldec commented Jan 20, 2023

Thanks @selfcontained
Works like a charm in the top level (tiled list of) projects page.
Can we do the same for the other links in the branches list as well?
(see original PR)

@roboquat roboquat added size/M and removed size/S labels Jan 21, 2023
geropl
geropl reviewed Jan 23, 2023
View reviewed changes
components/dashboard/src/projects/Project.tsx
href: gitpodHostUrl
.withContext(`${branch.url}`, { showOptions: true })
.toString(),
onClick: () =>
Copy link
Member

Choose a reason for hiding this comment

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

Like the preserving of context here!

geropl
geropl approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Code LGTM, and trust @jldec test. ✔️

@roboquat roboquat merged commit 973ef99 into main Jan 23, 2023
@roboquat roboquat deleted the bmh/allow-users-to-close-15696 branch January 23, 2023 10:43
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jan 23, 2023
@gtsiolis gtsiolis mentioned this pull request Jan 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
4 participants
@selfcontained @jldec @geropl @roboquat