Skip to content

[dashboard] misc dashboard improvements #3496

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

Merged
merged 3 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions components/dashboard/src/components/ContextMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useState } from 'react';
import { MouseEvent, useState } from 'react';

export interface ContextMenuProps {
children: React.ReactChild[] | React.ReactChild;
Expand All @@ -14,7 +14,7 @@ export interface ContextMenuEntry {
*/
separator?: boolean;
customFontStyle?: string;
onClick?: ()=>void;
onClick?: (event: MouseEvent)=>void;
href?: string;
}

Expand All @@ -34,9 +34,10 @@ function ContextMenu(props: ContextMenuProps) {
const enhancedEntries = props.menuEntries.map(e => {
return {
... e,
onClick: () => {
e.onClick && e.onClick();
onClick: (event: MouseEvent) => {
e.onClick && e.onClick(event);
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

toggleExpanded();
event.preventDefault();
}
}
})
Expand Down
42 changes: 36 additions & 6 deletions components/dashboard/src/components/Modal.tsx
Original file line number Diff line number Diff line change
@@ -1,27 +1,57 @@
import { Disposable, DisposableCollection } from "@gitpod/gitpod-protocol";
import { useEffect } from "react";

export default function Modal(props: {
children: React.ReactChild[] | React.ReactChild,
visible: boolean,
closeable?: boolean,
className?: string,
onClose: () => void
onClose: () => void,
onEnter?: () => boolean
}) {
const disposable = new DisposableCollection();
const close = () => {
disposable.dispose();
props.onClose();
}
useEffect(() => {
if (!props.visible) {
return;
}
const keyHandler = (k: globalThis.KeyboardEvent) => {
if (k.eventPhase === 1 /* CAPTURING */) {
if (k.key === 'Escape') {
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Modals do not close on ESC. 💭

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, the predicate for the capturing phase was wrong

close();
}
if (k.key === 'Enter') {
if (props.onEnter) {
if (props.onEnter() === false) {
return;
}
}
close();
k.stopPropagation();
}
}
}
window.addEventListener('keydown', keyHandler, { capture: true });
disposable.push(Disposable.create(()=> window.removeEventListener('keydown', keyHandler)));
});
if (!props.visible) {
return null;
}
setTimeout(() => window.addEventListener('click', props.onClose, { once: true }), 0);
return (
<div className="fixed top-0 left-0 bg-black bg-opacity-70 z-50 w-screen h-screen" >
<div className="fixed top-0 left-0 bg-black bg-opacity-70 z-50 w-screen h-screen" onClick={close}>
Copy link
Member

Choose a reason for hiding this comment

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

I believe, onClick={close} here will trigger close even if you click-select anything within the modal.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not, because I stop.propagation below

Copy link
Member

Choose a reason for hiding this comment

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

I see

<div className="w-screen h-screen align-middle" style={{display: 'table-cell'}}>
<div className={"relative bg-white border rounded-xl p-6 max-w-lg mx-auto text-gray-600" + props.className}>
<div className={"relative bg-white border rounded-xl p-6 max-w-lg mx-auto text-gray-600" + props.className} onClick={e => e.stopPropagation()}>
{props.closeable !== false && (
<div className="absolute right-9 top-8 cursor-pointer" onClick={props.onClose}>
<div className="absolute right-7 top-6 cursor-pointer hover:bg-gray-200 rounded-md p-2" onClick={close}>
<svg version="1.1" width="14px" height="14px"
viewBox="0 0 100 100">
<line x1="0" y1="0" x2="100" y2="100" stroke="currentColor" strokeWidth="10px" />
<line x1="0" y1="100" x2="100" y2="0" stroke="currentColor" strokeWidth="10px" />
</svg>
</div>

)}
{props.children}
</div>
Expand Down
112 changes: 61 additions & 51 deletions components/dashboard/src/workspaces/WorkspaceEntry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { MouseEvent, useState } from 'react';
import { WorkspaceModel } from './workspace-model';


export function WorkspaceEntry({desc, model}: {desc: WorkspaceInfo, model: WorkspaceModel}) {
export function WorkspaceEntry({ desc, model }: { desc: WorkspaceInfo, model: WorkspaceModel }) {
const [isModalVisible, setModalVisible] = useState(false);
const [isChangesModalVisible, setChangesModalVisible] = useState(false);
const state: WorkspaceInstancePhase = desc.latestInstance?.status?.phase || 'stopped';
Expand Down Expand Up @@ -46,14 +46,21 @@ export function WorkspaceEntry({desc, model}: {desc: WorkspaceInfo, model: Works
pathname: '/start/',
hash: '#' + ws.id
});
const downloadURL = new GitpodHostUrl(window.location.href).with({
pathname: `/workspace-download/get/${ws.id}`
const downloadURL = new GitpodHostUrl(window.location.href).with({
pathname: `/workspace-download/get/${ws.id}`
}).toString();
const menuEntries: ContextMenuEntry[] = [
{
title: 'Open',
href: startUrl.toString()
},
}];
if (state === 'running') {
menuEntries.push({
title: 'Stop',
onClick: () => getGitpodService().server.stopWorkspace(ws.id)
});
}
menuEntries.push(
{
title: 'Download',
href: downloadURL
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Download is not working yet, right? Is this related to #3275?

Expand All @@ -80,63 +87,66 @@ export function WorkspaceEntry({desc, model}: {desc: WorkspaceInfo, model: Works
setModalVisible(true);
}
}
];
);
const project = getProject(ws);
const startWsOnClick = (event: MouseEvent) => {
window.location.href = startUrl.toString();
}
const showChanges = (event: MouseEvent) => {
event.preventDefault();
setChangesModalVisible(true);
}
return <div className="whitespace-nowrap flex space-x-2 py-6 px-6 w-full justify-between hover:bg-gray-100 cursor-pointer rounded-xl">
<div className="pr-3 self-center" onClick={startWsOnClick}>
<div className={stateClassName}>
&nbsp;
return <div>
<a className="rounded-xl whitespace-nowrap flex space-x-2 py-6 px-6 w-full justify-between hover:bg-gray-100 cursor-pointer" href={startUrl.toString()}>
<div className="pr-3 self-center">
<div className={stateClassName}>
&nbsp;
</div>
</div>
<div className="flex flex-col w-3/12">
<div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Let's remove the underline here! I've added a hover state, could we also set the width here to fit-content or something similar since this is not fully supported by all browsers?

We could also remove the hover state on the workspace name all together. WDYT?

Suggested change
<div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div>
<div className="font-medium text-gray-800 hover:bg-gray-200 rounded-md relative -left-1 -top-1 px-1">{ws.id}</div>

<a href={project ? 'https://' + project : undefined}><div className="text-sm overflow-ellipsis truncate text-gray-400 truncate">{project || 'Unknown'}</div></a>
</div>
</div>
<div className="flex flex-col w-3/12" onClick={startWsOnClick}>
<div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div>
<a href={project ? 'https://'+ project : undefined}><div className="text-sm overflow-ellipsis truncate text-gray-400 truncate">{project || 'Unknown'}</div></a>
</div>
<div className="flex w-4/12 truncate overflow-ellipsis" onClick={startWsOnClick}>
<div className="flex flex-col">
<div className="font-medium text-gray-500 truncate">{ws.description}</div>
<div className="text-sm text-gray-400 truncate">{ws.contextURL}</div>
<div className="flex w-4/12 truncate overflow-ellipsis">
<div className="flex flex-col">
<div className="font-medium text-gray-500 truncate">{ws.description}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Let's make this less prominent.

Suggested change
<div className="font-medium text-gray-500 truncate">{ws.description}</div>
<div className="text-gray-500 truncate">{ws.description}</div>

<div className="text-sm text-gray-400 truncate">{ws.contextURL}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Shouldn't this be more descriptive like Branch master? For now it only replicates the repository URL, right?

</div>
</div>
<div className="flex w-2/12" onClick={numberOfChanges > 0 ? showChanges : undefined}>
<div className="flex flex-col">
<div className="font-medium text-gray-500 truncate">{currentBranch}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Let's make this less prominent.

Suggested change
<div className="font-medium text-gray-500 truncate">{currentBranch}</div>
<div className="text-gray-500 truncate">{currentBranch}</div>
BEFORE AFTER
image image

{
numberOfChanges > 0 ?
<div className={"text-sm text-red truncate cursor-pointer hover:underline"} onClick={showChanges}>{changesLabel}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of this hover state?

Suggested change
<div className={"text-sm text-red truncate cursor-pointer hover:underline"} onClick={showChanges}>{changesLabel}</div>
<div className={"text-sm text-red truncate cursor-pointer hover:bg-red-100 rounded-md relative -left-1 px-1"} onClick={showChanges}>{changesLabel}</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

question: Changes currently open a modal. What do you think of having a dropdown here as seen in the specs? If you think a modal fits better, let me know so that I can provide some design specs.

:
<div className="text-sm text-gray-400 truncate">No Changes</div>
}
</div>
</div>
</div>
<div className="flex w-2/12" onClick={numberOfChanges > 0 ? showChanges: startWsOnClick}>
<div className="flex flex-col">
<div className="font-medium text-gray-500 truncate">{currentBranch}</div>
{
numberOfChanges > 0 ?
<div className={"text-sm text-red truncate cursor-pointer hover:underline"} onClick={showChanges}>{changesLabel}</div>
:
<div className="text-sm text-gray-400 truncate">No Changes</div>
}
<Modal visible={isChangesModalVisible} onClose={() => setChangesModalVisible(false)}>
{getChangesPopup(pendingChanges)}
</Modal>
<div className="flex w-2/12 self-center space-x-2">
<div className="text-sm text-gray-400 truncate">{moment(WorkspaceInfo.lastActiveISODate(desc)).fromNow()}</div>
</div>
</div>
<div className="flex w-2/12 self-center space-x-2" onClick={startWsOnClick}>
<div className="text-sm text-gray-400 truncate">{moment(WorkspaceInfo.lastActiveISODate(desc)).fromNow()}</div>
</div>
<div className="flex w-8 self-center hover:bg-gray-300 rounded-md cursor-pointer">
<ContextMenu menuEntries={menuEntries}>
<img className="w-8 h-8 p-1" src={ThreeDots} alt="Actions" />
</ContextMenu>
</div>
<Modal visible={isModalVisible} onClose={() => setModalVisible(false)}>
<div className="flex w-8 self-center hover:bg-gray-300 rounded-md cursor-pointer">
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Minor hover state update.

Suggested change
<div className="flex w-8 self-center hover:bg-gray-300 rounded-md cursor-pointer">
<div className="flex w-8 self-center hover:bg-gray-200 rounded-md cursor-pointer">

<ContextMenu menuEntries={menuEntries}>
<img className="w-8 h-8 p-1" src={ThreeDots} alt="Actions" />
</ContextMenu>
</div>
</a>
<Modal visible={isChangesModalVisible} onClose={() => setChangesModalVisible(false)}>
{getChangesPopup(pendingChanges)}
</Modal>
<Modal visible={isModalVisible} onClose={() => setModalVisible(false)} onEnter={() => {model.deleteWorkspace(ws.id); return true;}}>
<div>
<h3>Delete {ws.id}</h3>
<div className="py-4">
<p>Do you really want to delete this workspace?</p>
<h3>Delete Workspace</h3>
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
<p className="mt-1 mb-2 text-base">Are you sure you want to delete this workspace?</p>
<div className="w-full p-4 mb-2 bg-gray-100 rounded-xl group bg-gray-100">
<p className="text-base text-gray-800 font-semibold">{ws.id}</p>
<p>{ws.description}</p>
</div>
</div>
<div className="flex">
<div className="flex-1"></div>
<div className="flex justify-end mt-5">
<button className="cursor-pointer px-3 py-2 text-white text-sm rounded-md border-2 border-red-800 bg-red-600 hover:bg-red-800"
onClick={()=>getGitpodService().server.deleteWorkspace(ws.id)}>
Delete
onClick={() => model.deleteWorkspace(ws.id)}>
Delete Workspace
</button>
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion components/dashboard/src/workspaces/Workspaces.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export class Workspaces extends React.Component<WorkspacesProps, WorkspacesState
<div className="w-3/12">Name</div>
<div className="w-4/12">Context</div>
<div className="w-2/12">Pending Changes</div>
<div className="w-2/12">Last Active</div>
<div className="w-2/12">Last Start</div>
Copy link
Member Author

@svenefftinge svenefftinge Mar 19, 2021

Choose a reason for hiding this comment

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

@gtsiolis I changed this to Last Start because that is what the time really shows. Last Active was confusing because for running workspaces it seems they should be active just now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was tempted to suggest to revert this but let's leave this as is for now. Seems more accurate indeed!

Copy link
Contributor

Choose a reason for hiding this comment

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

question: If creation timestamp is still important, could we add a tooltip when hovering on the last start timestamp with the creation timestamp (e.g. Created 2 days ago)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure it is important

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's drop it for now.

<div className="w-8"></div>
</div>
{
Expand Down
18 changes: 12 additions & 6 deletions components/dashboard/src/workspaces/workspace-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Disposable, DisposableCollection, GitpodClient, WorkspaceInfo, Workspac
import { getGitpodService } from "../service/service";

export class WorkspaceModel implements Disposable, Partial<GitpodClient> {

protected workspaces = new Map<string,WorkspaceInfo>();
protected currentlyFetching = new Set<string>();
protected disposables = new DisposableCollection();
Expand All @@ -15,7 +15,7 @@ export class WorkspaceModel implements Disposable, Partial<GitpodClient> {
constructor(protected setWorkspaces: (ws: WorkspaceInfo[]) => void) {
this.internalRefetch();
}

protected internalRefetch() {
this.disposables.dispose();
this.disposables = new DisposableCollection();
Expand All @@ -27,17 +27,17 @@ export class WorkspaceModel implements Disposable, Partial<GitpodClient> {
});
this.disposables.push(getGitpodService().registerClient(this));
}

protected updateMap(workspaces: WorkspaceInfo[]) {
for (const ws of workspaces) {
this.workspaces.set(ws.workspace.id, ws);
}
}

dispose(): void {
this.disposables.dispose();
}

async onInstanceUpdate(instance: WorkspaceInstance) {
if (this.workspaces) {
if (this.workspaces.has(instance.workspaceId)) {
Expand All @@ -58,7 +58,13 @@ export class WorkspaceModel implements Disposable, Partial<GitpodClient> {
}
}
}


async deleteWorkspace(id: string): Promise<void> {
await getGitpodService().server.deleteWorkspace(id);
this.workspaces.delete(id);
this.notifyWorkpaces();
}

protected internalActive = true;
get active() {
return this.internalActive;
Expand Down