Allow renaming tabs #1948
Allow renaming tabs #1948
Conversation
|
The renaming works, however some other part of the app sometimes throws an error after a document got renamed: graphite_wasm.js:1255 error
frontend/wasm/src/editor_api.rs:114While handling FrontendMessage ""UpdatePropertyPanelOptionsLayout"",
JavaScript threw an error: JsValue(TypeError: Cannot use 'in' operator to search for 'columnWidgets' in undefinedTypeError: Cannot use 'in' operator to search for 'columnWidgets' in undefined
at http://localhost:8080/src/wasm-communication/messages.ts:753:27
at Array.reduce (<anonymous>)
at http://localhost:8080/src/wasm-communication/messages.ts:752:42
at Array.forEach (<anonymous>)
at patchWidgetLayout (http://localhost:8080/src/wasm-communication/messages.ts:751:16)
at http://localhost:8080/src/components/panels/Properties.svelte?t=1725132032461:286:4
at callCallback (http://localhost:8080/src/wasm-communication/subscription-router.ts:25:9) at Object.handleJsMessage (http://localhost:8080/src/wasm-communication/subscription-router.ts:33:5) at http://localhost:8080/src/wasm-communication/editor.ts:18:19
at http://localhost:8080/wasm/pkg/graphite_wasm.js:2388:37) |
tmp: placeholder for renameAction panel: newName first as arg allow renaming document tabs
|
Sorry, in bed yesterday I noticed that the renaming should be cancel-able. The PR notifications remembered me to implement that |
| @@ -182,6 +218,10 @@ | |||
| align-items: center; | |||
| position: relative; | |||
|
|
|||
| > button { | |||
| all: unset; | |||
There was a problem hiding this comment.
I think you accidentally disabled styling on the cross button with this.
| @@ -87,7 +104,26 @@ | |||
| }} | |||
| bind:this={tabElements[tabIndex]} | |||
| > | |||
| <TextLabel>{tabLabel.name}</TextLabel> | |||
| {#if isRenaming} | |||
| <input | |||
There was a problem hiding this comment.
The styling on this should probably match when renaming layers (e.g. no outline etc.).
| <TextLabel>{tabLabel.name}</TextLabel> | ||
| {#if isRenaming} | ||
| <input | ||
| type="text" |
There was a problem hiding this comment.
The default value for the input should probably be the current tab value rather than being empty.
|
It may be a few days before I can commit some changes that you can rely on to fix this so it behaves the same as layer renaming. I need to focus on other code priorities but I'll try to do that soon. |
|
@Keavon what is your status on this? |
|
I haven't had time yet. It's probably a couple hours of work, but this feature isn't a priority since it adds no new functionality, so it hasn't been where I've put my focus in the past days. I'll try to avoid letting this rot for too long though. |
|
Actually, on further thought, the hard part isn't what's done by this PR— it's the generalizing of the existing components. It requires unifying how the TextInput and TextLabel can behave like one another to support the exact renaming behavior seen currently, and as of yet exclusively, for layers in the Layers panel. Since, unfortunately, this PR doesn't take that approach, it isn't suitable for merging in its current state. But once I'm able to unify the behaviors of those two components and replace how it's used in the Layers panel, adding it to the document tab and sending that to the backend is about 10 minutes of work and would probably only use a few lines of code present in this PR. So it wouldn't actually make much sense to keep this PR around and reuse it when I have time for that, whenever that might be. The reality is that this is just a tricky frontend task that seems simple but isn't, so I don't suggest having it as a community-contributed task. Hopefully you got to learn a bit about the backend <-> frontend communication in this process, and sorry to waste your time with this @vdawg-git. |
|
All good and thank you for the detailed explanation |
Closes #1854