Skip to content

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

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

Merged
merged 1 commit into from
Jan 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 201 additions & 0 deletions components/dashboard/src/components/DropDown2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
/**
* Copyright (c) 2022 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import React, { FunctionComponent, RefObject, useCallback, useEffect, useMemo, useRef, useState } from "react";
import Arrow from "./Arrow";

export interface DropDown2Element {
id: string;
element: JSX.Element;
isSelectable?: boolean;
}

export interface DropDown2Props {
getElements: (searchString: string) => DropDown2Element[];
searchPlaceholder?: string;
disableSearch?: boolean;
expanded?: boolean;
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.

const [showDropDown, setShowDropDown] = useState<boolean>(!!props.expanded);
const nodeRef: RefObject<HTMLDivElement> = useRef(null);
const onSelected = useCallback(
(elementId: string) => {
props.onSelectionChange(elementId);
setShowDropDown(false);
},
[props],
);
const [search, setSearch] = useState<string>("");
const filteredOptions = useMemo(() => props.getElements(search), [props, search]);
const [selectedElementTemp, setSelectedElementTemp] = useState<string | undefined>(filteredOptions[0]?.id);

// reset search when the drop down is expanded or closed
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.

}
// we only want this behavior when showDropDown changes to true.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [showDropDown]);

const toggleDropDown = useCallback(() => {
setShowDropDown(!showDropDown);
}, [setShowDropDown, showDropDown]);

const setFocussedElement = useCallback(
(element: string) => {
setSelectedElementTemp(element);
document.getElementById(element)?.scrollIntoView({ behavior: "smooth", block: "nearest" });
document.getElementById(element)?.focus();
},
[setSelectedElementTemp],
);

const onKeyDown = useCallback(
(e: React.KeyboardEvent) => {
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?

const candidate = filteredOptions[idx];
if (candidate.isSelectable) {
setFocussedElement(candidate.id);
return;
}
}
return;
}
if (showDropDown && e.key === "ArrowUp") {
e.preventDefault();
let idx = filteredOptions.findIndex((e) => e.id === selectedElementTemp);
while (idx-- > 0) {
const candidate = filteredOptions[idx];
if (candidate.isSelectable) {
setFocussedElement(candidate.id);
return;
}
}
return;
}
if (showDropDown && e.key === "Escape") {
setShowDropDown(false);
e.preventDefault();
}
if (e.key === "Enter") {
if (showDropDown && selectedElementTemp && filteredOptions.some((e) => e.id === selectedElementTemp)) {
e.preventDefault();
props.onSelectionChange(selectedElementTemp);
setShowDropDown(false);
}
if (!showDropDown) {
toggleDropDown();
e.preventDefault();
}
}
if (e.key === " ") {
toggleDropDown();
e.preventDefault();
}
},
[filteredOptions, props, selectedElementTemp, setFocussedElement, showDropDown, toggleDropDown],
);

const handleBlur = useCallback(
(e: React.FocusEvent) => {
// postpone a little, so it doesn't fire before a click event for the main element.
setTimeout(() => {
// only close if the focussed element is not child
if (!nodeRef?.current?.contains(window.document.activeElement)) {
setShowDropDown(false);
}
}, 100);
},
[setShowDropDown],
);

return (
<div
onKeyDown={onKeyDown}
onBlur={handleBlur}
ref={nodeRef}
tabIndex={0}
className={"relative flex flex-col rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-300"}
>
<div
className={
"h-16 bg-gray-100 dark:bg-gray-800 flex items-center px-2 " +
(showDropDown
? "rounded-t-lg"
: "rounded-lg hover:bg-gray-200 dark:hover:bg-gray-700 cursor-pointer")
}
onClick={toggleDropDown}
>
{props.children}
<div className="flex-grow" />
<div className="mr-2">
<Arrow direction={showDropDown ? "up" : "down"} />
</div>
</div>
{showDropDown && (
<>
<div className="absolute w-full top-12 bg-gray-100 dark:bg-gray-800 max-h-72 overflow-auto rounded-b-lg mt-3 z-50 p-2">
{!props.disableSearch && (
<div className="h-12">
<input
type="text"
autoFocus
className={"w-full focus rounded-lg"}
placeholder={props.searchPlaceholder}
value={search}
onChange={(e) => setSearch(e.target.value)}
/>
</div>
)}
<ul>
{filteredOptions.length > 0 ? (
filteredOptions.map((element) => {
let selectionClasses = `dark:bg-gray-800 cursor-pointer`;
if (element.id === selectedElementTemp) {
selectionClasses = `bg-gray-200 dark:bg-gray-700 cursor-pointer focus:outline-none focus:ring-0`;
}
if (!element.isSelectable) {
selectionClasses = ``;
}
return (
<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".

className={"h-16 rounded-lg flex items-center px-2 " + selectionClasses}
onMouseDown={() => {
if (element.isSelectable) {
setFocussedElement(element.id);
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.

onFocus={() => setFocussedElement(element.id)}
>
{element.element}
</li>
);
})
) : (
<li key="no-elements" className={"rounded-md "}>
<div className="h-12 pl-8 py-3 text-gray-800 dark:text-gray-200">No results</div>
</li>
)}
</ul>
</div>
</>
)}
</div>
);
};
2 changes: 1 addition & 1 deletion components/dashboard/src/components/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ type ModalBodyProps = {
export const ModalBody = ({ children, hideDivider = false }: ModalBodyProps) => {
return (
<div
className={cn("overflow-y-auto border-gray-200 dark:border-gray-800 -mx-6 px-6 ", {
className={cn("border-gray-200 dark:border-gray-800 -mx-6 px-6 ", {
"border-t border-b mt-2 py-4": !hideDivider,
})}
>
Expand Down
Loading