[dashboard] Allow workspace renaming #5695

laushinka · 2021-09-14T13:36:55Z

Description

After a discussion with @corneliusludmann and @gtsiolis, instead of the initial proposal of allowing workspace ID renames, this PR allows the renaming of a workspace context description.
The reason to go this way is because the the workspaceID affects at the very least the URL of the workspace, and therefore could introduce breaking changes.

Related Issue(s)

Fixes #3946

How to test

Go to list of workspaces in Dashboard.
Click on the options on the far right, choose Rename.
Give the workspace a different description.

Release Notes

Allows renaming of workspace description.

gtsiolis · 2021-09-15T15:17:28Z

Looking at this now! 👀

gtsiolis

UX looks great overall, @laushinka! 🌟

Lunch happened and reviewing took longer than expected! 🍝

Left some comments below, but feel free to open follow up issues for anything that would significantly increase the scope of this PR. 💭

Approving to unblock merging. I'd suggest to also ask for a closer look (review) at the code. ➿

/hold

gtsiolis · 2021-09-15T15:18:48Z

components/dashboard/src/workspaces/WorkspaceEntry.tsx

 import ConfirmationModal from '../components/ConfirmationModal';
+import Modal from '../components/Modal';


praise: Good to see the modal component in action again! ➿

gtsiolis · 2021-09-15T15:23:26Z

components/dashboard/src/workspaces/WorkspaceEntry.tsx

+        <Modal visible={isRenameModalVisible} onClose={() => setRenameModalVisible(false)} onEnter={() => { updateWorkspaceDescription(); return true }}>
+            <h3 className="mb-4">Rename Workspace Description</h3>
+            <div className="border-t border-b border-gray-200 dark:border-gray-800 -mx-6 px-6 py-4 space-y-2">
+                <input className="w-full" type="text" defaultValue={workspaceDescription} ref={renameInputRef} />


issue: When removing the description text, could we use a placeholder to fall back to the previous description. If the initial context name is not available we could reuse the last one in place.

Using an empty string Changed description Removed description

Good catch!

@laushinka Alternatively, thinking about this again, a boring (simple) solution could be to not allow empty descriptions and return a warning alert which is similar to what we do with variables.💡

nitpick: Nice! What do you think of using a softer tone like the following?

Description cannot be empty.

gtsiolis · 2021-09-15T16:29:54Z

components/dashboard/src/workspaces/WorkspaceEntry.tsx

+        <Modal visible={isRenameModalVisible} onClose={() => setRenameModalVisible(false)} onEnter={() => { updateWorkspaceDescription(); return true }}>
+            <h3 className="mb-4">Rename Workspace Description</h3>
+            <div className="border-t border-b border-gray-200 dark:border-gray-800 -mx-6 px-6 py-4 space-y-2">
+                <input className="w-full" type="text" defaultValue={workspaceDescription} ref={renameInputRef} />


suggestion: Could we enforce a character limit let's say of 250 characters as a) it does not make sense to allow more characters since these won't be visible in the dashboard and b) the generic alert dialog when saving with a long description does not provide any useful context, right?

Long description name hidden in the dashboard Long description name error

components/dashboard/src/workspaces/WorkspaceEntry.tsx

gtsiolis · 2021-09-15T16:41:48Z

components/dashboard/src/workspaces/WorkspaceEntry.tsx

+        <Modal visible={isRenameModalVisible} onClose={() => setRenameModalVisible(false)} onEnter={() => { updateWorkspaceDescription(); return true }}>
+            <h3 className="mb-4">Rename Workspace Description</h3>
+            <div className="border-t border-b border-gray-200 dark:border-gray-800 -mx-6 px-6 py-4 space-y-2">
+                <input className="w-full" type="text" defaultValue={workspaceDescription} ref={renameInputRef} />


issue: Long descriptions seems to bring back a similar overflow issue as in #4556.

Overflowing text when deleting a workspace

We should truncate it in addition to the 250 limit I suppose?

I'd vote for truncation to also avoid breaking this component!

On the other side, we could go all-in by a) wrapping the text, b) allowing longer description to surface on the dashboard, etc. but this would be against the notion of ephemeral environments. Maybe this is something to consider in future if needed. 🗳️

Pushed a solution to show an error message and also truncating it. Let me know of a better error message! I'm off tomorrow, so feel free to change it here as well if I don't see the follow-up comment today.

components/dashboard/src/workspaces/WorkspaceEntry.tsx

gtsiolis · 2021-09-16T08:10:13Z

components/dashboard/src/workspaces/WorkspaceEntry.tsx

@@ -96,7 +128,7 @@ export function WorkspaceEntry({ desc, model, isAdmin, stopWorkspace }: Props) {
            </Tooltip>
        </ItemField>
        <ItemField className="w-4/12 flex flex-col">
-            <div className="text-gray-500 dark:text-gray-400 overflow-ellipsis truncate">{ws.description}</div>
+            <div className="text-gray-500 dark:text-gray-400 overflow-ellipsis truncate">{workspaceDescription}</div>


thought: Thinking of this PR and since this was a feature requested from some community members would it be interesting to measure how much people use this feature? Maybe have a self-serve graph of how many of the existing workspaces are renamed per day or week? Maybe also how many times people clicked the rename option in the dropdown option? It could be interesting to also see if the usage of this feature decreases after #3594. What do you think? Feel free to drop the idea if this would increase the scope of these changes. 💭

Cc @jakobhero

fyi: Discussed this briefly with @svenefftinge earlier in the product sync meeting and we decided that adding tracking events here is a low priority given that the renaming functionality will be hidden behind the more actions dropdown. Up to you @laushinka and @jakobhero to decide if it's worth adding any tracking events within the scope of this PR. 🏓

Agree that it would be useful to keep track but that we could do it in a separate issue if @jakobhero also thinks it's worth to track 🙏🏽

hey there!👋 all clicks on buttons, anchors or clickable divs in the dashboard will be automatically tracked with the project & teams tracking PR i am currently working on. this means that we will see how many people clicked on the rename button, but now how many renames were successfully performed (which probably would be best tracked on the server). i think having visibility on how many users clicked on the button to open the modal is a good start to see how important this is to users and sufficient for now, wdyt?

Fixes #3946

laushinka · 2021-09-16T14:25:45Z

/unhold

@gtsiolis @corneliusludmann @JanKoehnlein just an fyi that I'll be off tomorrow.

JanKoehnlein · 2021-09-16T14:37:38Z

/lgtm

JanKoehnlein · 2021-09-16T14:54:55Z

/lgtm

roboquat · 2021-09-16T14:55:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtsiolis, JanKoehnlein

Associated issue: #3946

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/dashboard/OWNERS~~ [JanKoehnlein,gtsiolis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

roboquat added release-note do-not-merge/work-in-progress labels Sep 14, 2021

roboquat requested a review from JanKoehnlein September 14, 2021 13:37

laushinka removed the request for review from JanKoehnlein September 14, 2021 13:37

roboquat added the size/M label Sep 14, 2021

laushinka force-pushed the laushinka/rename-workspace-3946 branch 2 times, most recently from 51862a5 to 801e327 Compare September 15, 2021 10:46

laushinka removed the do-not-merge/work-in-progress label Sep 15, 2021

laushinka requested review from gtsiolis, corneliusludmann and JanKoehnlein September 15, 2021 10:54

laushinka marked this pull request as ready for review September 15, 2021 10:56

gtsiolis approved these changes Sep 15, 2021

View reviewed changes

roboquat added do-not-merge/hold approved labels Sep 15, 2021

gtsiolis reviewed Sep 16, 2021

View reviewed changes

laushinka force-pushed the laushinka/rename-workspace-3946 branch from 2b9152c to 7f3b634 Compare September 16, 2021 10:25

[dashboard] Allow workspace renaming

e53e486

Fixes #3946

laushinka force-pushed the laushinka/rename-workspace-3946 branch from 0a2baf8 to e53e486 Compare September 16, 2021 14:06

roboquat removed the do-not-merge/hold label Sep 16, 2021

Update texts

170f8da

JanKoehnlein added the lgtm label Sep 16, 2021

roboquat merged commit d1a0264 into main Sep 16, 2021

roboquat deleted the laushinka/rename-workspace-3946 branch September 16, 2021 14:56

gtsiolis mentioned this pull request Oct 23, 2021

Focus on the first modal input when renaming a workspace #6371

Closed

iQQBot mentioned this pull request Oct 26, 2021

[dashboard] Add autofocus to some modal #6408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dashboard] Allow workspace renaming #5695

[dashboard] Allow workspace renaming #5695

laushinka commented Sep 14, 2021 •

edited

Loading

gtsiolis commented Sep 15, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Sep 15, 2021

gtsiolis Sep 15, 2021

laushinka Sep 16, 2021

gtsiolis Sep 16, 2021 •

edited

Loading

laushinka Sep 16, 2021

gtsiolis Sep 16, 2021

laushinka Sep 16, 2021

gtsiolis Sep 15, 2021

gtsiolis Sep 15, 2021

laushinka Sep 16, 2021 •

edited

Loading

gtsiolis Sep 16, 2021

laushinka Sep 16, 2021

gtsiolis Sep 16, 2021

gtsiolis Sep 16, 2021

laushinka Sep 16, 2021

jakobhero Sep 17, 2021

laushinka commented Sep 16, 2021

JanKoehnlein commented Sep 16, 2021

JanKoehnlein commented Sep 16, 2021

roboquat commented Sep 16, 2021

		import ConfirmationModal from '../components/ConfirmationModal';
		import Modal from '../components/Modal';

[dashboard] Allow workspace renaming #5695

[dashboard] Allow workspace renaming #5695

Conversation

laushinka commented Sep 14, 2021 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

gtsiolis commented Sep 15, 2021 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka commented Sep 16, 2021

JanKoehnlein commented Sep 16, 2021

JanKoehnlein commented Sep 16, 2021

roboquat commented Sep 16, 2021

laushinka commented Sep 14, 2021 •

edited

Loading

gtsiolis commented Sep 15, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Sep 16, 2021 •

edited

Loading

laushinka Sep 16, 2021 •

edited

Loading