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

[dashboard] Allow options on ws start (2/2) #15389

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Conversation

svenefftinge
Copy link
Member

@svenefftinge svenefftinge commented Dec 16, 2022

Description

Updates the Open Workspace Modal to allow specifying the workspace class and editor.

Screenshot 2022-12-20 at 09 58 53

Related Issue(s)

Fixes #15287

How to test

Use the Open workspace dialog on the preview's /workspaces view

Release Notes

Allow picking workspace class and editor when opening a workspace from the dashboard

Documentation

Werft options:

  • /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

@svenefftinge svenefftinge changed the title [WIP] [server] Allow options on ws start [WIP] [server] Allow options on ws start (2/2) Dec 16, 2022
@svenefftinge svenefftinge changed the title [WIP] [server] Allow options on ws start (2/2) [WIP] [dashboard] Allow options on ws start (2/2) Dec 16, 2022
@roboquat roboquat added size/XL and removed size/M labels Dec 16, 2022
@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch from e56ef18 to d016608 Compare December 17, 2022 10:38
@roboquat roboquat added size/XXL and removed size/XL labels Dec 17, 2022
@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 19, 2022

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.3
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Dec 19, 2022

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.4
(with .werft/ from main)

@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 19, 2022

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.5
(with .werft/ from main)

@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch from d016608 to c19a090 Compare December 19, 2022 14:28
@gtsiolis gtsiolis mentioned this pull request Dec 19, 2022
4 tasks
@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch 2 times, most recently from 18c07ed to bfefd13 Compare December 19, 2022 16:38
@svenefftinge svenefftinge changed the title [WIP] [dashboard] Allow options on ws start (2/2) [dashboard] Allow options on ws start (2/2) Dec 19, 2022
@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 19, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-start-with-options-2.9
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Dec 19, 2022

Got some minutes in front of the computer. Taking a quick look! 👀

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-start-with-options-2.10
(with .werft/ from main)

@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch 4 times, most recently from 03ba412 to d62e2c0 Compare December 20, 2022 08:39
@svenefftinge svenefftinge marked this pull request as ready for review December 20, 2022 08:57
@svenefftinge svenefftinge requested review from a team December 20, 2022 08:57
@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 20, 2022

filtering does not seem to kick in sometimes for editors

The filtering is based on '${ide.label}${ide.title}${ide.notes}${ide.id}'.toLowerCase().indexOf(search.toLowerCase()) !== -1. I guess the code entries somewhere have in within the description or so? 😅

Rider and CLion does not have latest options for some reasons

Some IDEs have the same version for latestImageVersion and imageVersion. In those cases I don't display the 'latest' option. Is that ok?

Description is going to be different in production?

Yes, it's just the preview env configuration that is different to prod (not really helpful, though).

drop-down does not collapse then another selected

Yes, also the ESC and cursor keys aren't working on the workspaceClass element. It is because it doesn't have a search input field. cc @selfcontained any idea how this could be fixed?

@andreafalzetti
Copy link
Contributor

andreafalzetti commented Dec 20, 2022

Dark mode select and modal look great now ✨ 🙏

I've noticed a weird behavioiur with the workspace class select:

2022-12-20 13 23 58

@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch 2 times, most recently from f1fcb4e to 934fda5 Compare December 20, 2022 14:26
@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 20, 2022

Yes, also the ESC and cursor keys aren't working on the workspaceClass element. It is because it doesn't have a search input field. cc @selfcontained any idea how this could be fixed?

I've fixed those issues, by moving the search input out of the view 😇 .

@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 20, 2022

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.18
(with .werft/ from main)

Copy link
Contributor

@selfcontained selfcontained left a comment

Choose a reason for hiding this comment

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

Most of the comments are non-blocking. I did notice some weird behavior trying to close expanded dropdowns (video included) that would be nice to fix.

Generally I had a comment about the level of effort it might take to make a component like this accessible, and how we might want to think about that.

components/dashboard/src/components/DropDown2.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/DropDown2.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/DropDown2.tsx Outdated Show resolved Hide resolved
components/dashboard/src/start/CreateWorkspace.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/SelectIDEComponent.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/SelectIDEComponent.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/SelectIDEComponent.tsx Outdated Show resolved Hide resolved
@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 29, 2022

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.19
(with .werft/ from main)

@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 29, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-start-with-options-2.20
(with .werft/ from main)

@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 29, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-start-with-options-2.22
(with .werft/ from main)

@svenefftinge
Copy link
Member Author

svenefftinge commented Dec 29, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-start-with-options-2.23
(with .werft/ from main)

@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch from 934fda5 to acea6bf Compare December 29, 2022 14:14
@svenefftinge svenefftinge force-pushed the se/start-with-options-2 branch from acea6bf to 699e02b Compare December 29, 2022 16:00
@filiptronicek
Copy link
Member

I think this would be very useful to have inside of https://github.com/gitpod-io/browser-extension replacing the Gitpod direct workspace opening button with a dropdown for all of the options; that's at least how I mainly open my workspaces.

@svenefftinge
Copy link
Member Author

I think this would be very useful to have inside of https://github.com/gitpod-io/browser-extension replacing the Gitpod direct workspace opening button with a dropdown for all of the options; that's at least how I mainly open my workspaces.

My current plan is to enhance the create workspace URL with a parameter that opens let's users review and change those options on before workspace creation.

@svenefftinge
Copy link
Member Author

svenefftinge commented Jan 3, 2023

/werft run

👍 started the job as gitpod-build-se-start-with-options-2.26
(with .werft/ from main)

Copy link
Contributor

@selfcontained selfcontained left a comment

Choose a reason for hiding this comment

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

This feels much better, thank you! I left a few non-blocking comments/ideas on other improvements we could make or consider. I don't think they have to hold it up though, and we could iterate on it as we go.

onSelectionChange: (id: string) => void;
}

export const DropDown2: FunctionComponent<DropDown2Props> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking - but I think it would be helpful in the future to have a better name for this component to help explain what it's intended for, or know when to use this vs. Dropdown. I don't have any good suggestions though, but thought I'd mention it in case you had any ideas.

useEffect(() => {
setSearch("");
if (showDropDown && selectedElementTemp) {
document.getElementById(selectedElementTemp)?.scrollIntoView({ behavior: "smooth", block: "nearest" });
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a non-blocking comment, but something to maybe think about as a way to improve the UX of this component. While this behavior is nice in that it helps show a selected option - it feels a bit disorienting to watch a long list scroll when you open the dropdown. It also adds an additional step if you just wanted to type to filter after you expanded the list (now the search input is not focused and out of view).

A more common UX pattern for interfaces like this I think that can fit well would be to have the selected component always show up on top of the list of options, and then the search input can stay focused when the dropdown opens, making it easy to type, and you also see the selected option.

Another option could be having the search input not in the scrollable container, and stay pinned so that only the options scroll. Either way, I don't think this is a blocker, just some suggestions to consider if we want to improve it a bit.

if (showDropDown && e.key === "ArrowDown") {
e.preventDefault();
let idx = filteredOptions.findIndex((e) => e.id === selectedElementTemp);
while (idx++ < filteredOptions.length - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me awhile to understand why we needed a loop here. I think it's so that we select the next "selectable" option, since not all options are selectable. If I'm understanding that correctly, maybe a comment to help future-us understand it a bit would be helpful?

<li
key={element.id}
id={element.id}
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to avoid having these option elements be tab indexed. As it is, once the container is open, tabbing doesn't proceed to the next "input" in the view, i.e. the next dropdown in this case, but shifts the selected option to the next one. A normal select in the browser will only use arrow keys for navigating the selected option, while tab will let you move on to the next "input".

onSelected(element.id);
}
}}
onMouseOver={() => setFocussedElement(element.id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part felt a little weird to me, having the mouseover select the option rather than requiring a click or keyboard navigation to select it. It also seems to be making the scroll container do a bit of a jittery scroll as you mouse over options at the edge of what's currently in view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note size/XXL team: SID team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start workspace with workspace class override
8 participants