[dashboard] improved workspaces #3457
Conversation
|
There is a secret link for this in the Reviewers section:
|
svenefftinge
force-pushed
the
minor-updates
branch
from
d003546 to
ba6d8fe
Compare
| @@ -5,6 +5,7 @@ | |||
| "private": true, | |||
| "dependencies": { | |||
| "@gitpod/gitpod-protocol": "0.1.5", | |||
| "moment": "^2.29.1", | |||
There was a problem hiding this comment.
Moment.js is a legacy project, now in maintenance mode. In most cases, you should choose a different library.
nit: it would be great to find an alternative to format relative time. this is quite a balloon in the resulting bundle.
There was a problem hiding this comment.
if it's used just to format relative time, maybe it's good to use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/RelativeTimeFormat, and there is a polyfill available, too.
| @@ -1,3 +1,3 @@ | |||
| export default function Separator() { | |||
There was a problem hiding this comment.
Is this component really necessary?
| </div> | ||
| <div className="flex flex-col w-3/12"> | ||
| <div className="text-gray-900">{ws.id}</div> | ||
| <div className="text-sm overflow-ellipsis truncate text-gray-400">{getProject(ws)}</div> | ||
| <Link to={"/start/#"+ws.id}><div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div></Link> |
There was a problem hiding this comment.
Maybe re-use startUrl?
| <Link to={"/start/#"+ws.id}><div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div></Link> | |
| <Link to={startUrl}><div className="font-medium text-gray-800 truncate hover:underline">{ws.id}</div></Link> |
| <div className="text-sm text-gray-400">{pendingChanges.toString() || 'No Changes'}</div> | ||
| <div className="font-medium text-gray-500 truncate">{currentBranch}</div> | ||
| <div className={changesClassName + " truncate cursor-pointer"} onClick={() => setChangesModalVisible(true)}>{changesLabel}</div> | ||
| <Modal visible={changesModalVisible} onClose={() => setChangesModalVisible(false)}> |
There was a problem hiding this comment.
Maybe rename changesModalVisible to isPendingChangesModalVisible?
| </div> | ||
| <Modal visible={modalVisible} onClose={() => setModalVisible(false)}> |
There was a problem hiding this comment.
Maybe rename modalVisible to isDeleteWorkspaceModalVisible?
| <DropDown entries={[{ | ||
| title: 'Status: Active', | ||
| onClick: onActive | ||
| }, { | ||
| title: 'Status: Recent', | ||
| onClick: onRecent | ||
| }]} /> |
There was a problem hiding this comment.
As a user, I have no idea what these mean, and they're not even mutually exclusive.
I still vote for:
Top Workspaces(by default, only running or pinned)All Workspaces(if you didn't find what you wanted)
|
Todos:
|
| <div className="bg-transparent h-1/3" /> | ||
| <div className="bg-white rounded-md px-6 py-4 max-w-lg mx-auto"> | ||
| <div className="bg-white border rounded-lg px-6 py-4 max-w-lg mx-auto"> |
|
/werft run 👍 started the job as gitpod-build-minor-updates.2 |
| </div> | ||
| </div> | ||
| <div className="lg:px-28 px-10 pt-4 flex flex-col space-y-2"> | ||
| <div className="py-2 flex justify-between space-x-2 text-sm text-gray-400 border-t border-b border-gray-200"> |
There was a problem hiding this comment.
nitpick: Minor spacing issue:
| <div className="py-2 flex justify-between space-x-2 text-sm text-gray-400 border-t border-b border-gray-200"> | |
| <div className="py-3 flex justify-between space-x-2 text-sm text-gray-400 border-t border-b border-gray-200"> | |
| {props.children} | ||
| </div> | ||
| {expanded? | ||
| <div className={`z-50 w-40 bg-white absolute py-2 right-0 flex flex-col border border-gray-300 rounded-lg space-y-2`}> |
There was a problem hiding this comment.
nitpick: Border color.
| <div className={`z-50 w-40 bg-white absolute py-2 right-0 flex flex-col border border-gray-300 rounded-lg space-y-2`}> | |
| <div className={`z-50 w-40 bg-white absolute py-2 right-0 flex flex-col border border-gray-200 rounded-lg space-y-2`}> |
| d="m280,278a153,153 0 1,0-2,2l170,170m-91-117 110,110-26,26-110-110" /> | ||
| </svg> | ||
| </div> | ||
| <input className="border-0 text-sm text-gray-400" type="text" placeholder="Search Workspace" onChange={(v) => { console.log(v) }} /> |
There was a problem hiding this comment.
issue: Could we set the placeholder color to gray-400 and user input with gray-600? 🎨
| </div></>; | ||
| <Header title="Workspaces" subtitle="Manage past workspaces" /> | ||
|
|
||
| <div className="lg:px-28 px-10 pt-4 flex"> |
There was a problem hiding this comment.
nitpick: Minor padding issue.
| <div className="lg:px-28 px-10 pt-4 flex"> | |
| <div className="lg:px-28 px-10 pt-8 flex"> |
| }]} /> | ||
| </div> | ||
| </div> | ||
| <div className="lg:px-28 px-10 pt-4 flex flex-col space-y-2"> |
There was a problem hiding this comment.
nitpick: Minor padding issue:
| <div className="lg:px-28 px-10 pt-4 flex flex-col space-y-2"> | |
| <div className="lg:px-28 px-10 flex flex-col space-y-2"> |
| <div className={`z-50 w-40 bg-white absolute py-2 right-0 flex flex-col border border-gray-300 rounded-lg space-y-2`}> | ||
| {enhancedEntries.map(e => { | ||
| const entry = <p key={e.title} className={`px-4 py-2 text-gray-600 hover:bg-gray-200 hover:text-gray-800 ${e.customFontStyle || font} ${e.separator? ' border-b border-gray-300':''}`} > | ||
| {e.title}{e.active ? <span className="pl-1 font-semibold">✓</span> : null} |
There was a problem hiding this comment.
question: Could we align the checkmark to the right end of the dropdown option?
| {e.title}{e.active ? <span className="pl-1 font-semibold">✓</span> : null} | ||
| </p> | ||
| return <a href={e.href} onClick={e.onClick}> | ||
| {entry} |
There was a problem hiding this comment.
quesrtion: Also, could we change the text and dropdown arrow icon color to gray-600 on hover?
There was a problem hiding this comment.
Could you clarify, I think it is already gray-600 without hover.
There was a problem hiding this comment.
I remember seeing the status filter dropdown label and arrow using the same color on hover. Ideally both the label and the icon arrow would use gray-400 and switch to gray-600 on hover. Feel free to ignore this if this is not the case anymore. 🏓
| break; | ||
| } | ||
| case 'interrupted': { | ||
| stateClassName += ' bg-red-200 text-red-600' | ||
| stateClassName += ' bg-red-400 text-red-600' |
There was a problem hiding this comment.
question: What do you think of adding the kumquat color (#FFB45B) for the starting phase?
| } | ||
| ]; | ||
| const project = getProject(ws); | ||
| return <div className="whitespace-nowrap flex space-x-2 py-6 w-full justify-between hover:bg-gray-100 rounded-lg"> |
There was a problem hiding this comment.
nitpick: Left and right padding here is 24px (px-6). Then we can remove the inner left padding below in https://github.com/gitpod-io/gitpod/pull/3457/files#diff-02ffe6851cca7925757e521dfba549b1e72892581cc6a6596cb4f5c0656cb300R91.
There was a problem hiding this comment.
thought: Let's also add cursor-pointer and change radius to rounded-xl (12px). @jankeromnes added the link on the row in the running workspaces modal and feels good! WDYT?
| ]; | ||
| const project = getProject(ws); | ||
| return <div className="whitespace-nowrap flex space-x-2 py-6 w-full justify-between hover:bg-gray-100 rounded-lg"> | ||
| <div className="w-10 self-center pl-4" > |
There was a problem hiding this comment.
nitpick: Minor padding issue:
| <div className="w-10 self-center pl-4" > | |
| <div className="pr-3 self-center" > |
svenefftinge
force-pushed
the
minor-updates
branch
2 times, most recently
from
8e9ea76 to
59a0719
Compare
|
/werft run 👍 started the job as gitpod-build-minor-updates.5 |
svenefftinge
force-pushed
the
minor-updates
branch
from
59a0719 to
7bab1e8
Compare
[dashboard] improved workspaces
[dashboard] improved workspaces
[dashboard] improved workspaces
Ready for a review.