-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(rename-sql-editor-tabs): Allows renaming of tabs #27608
base: master
Are you sure you want to change the base?
Conversation
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
return ( | ||
<button | ||
<div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transitioned to <div />
, since we were throwing a nested button warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editing looks good. Some comments on regressions of how view states/namings work
}} | ||
/> | ||
) : ( | ||
<div onClick={() => setIsEditing(!isEditing)} className="flex-grow text-left whitespace-pre"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this something like dblclick? It's a bit too easy to end up in the editing mode (even when just navigating between tabs)
/> | ||
) : ( | ||
<div onClick={() => setIsEditing(!isEditing)} className="flex-grow text-left whitespace-pre"> | ||
{tabName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to keep model.view?.name
as a possible value here. If the tab is editing a view, the tab name probably shouldn't be editable. (We want view names to be static effectively as we need to guarantee uniqueness.)
@@ -339,7 +361,7 @@ export const multitabEditorLogic = kea<multitabEditorLogicType>([ | |||
return { | |||
query: props.monaco?.editor.getModel(model.uri)?.getValue() || '', | |||
path: model.uri.path.split('/').pop(), | |||
view: model.view, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a mis delete? We'll want the view to still be here otherwise the views won't persist between refreshes
onClick={() => onClick?.(model)} | ||
className={clsx( | ||
'space-y-px rounded-t p-1 flex flex-row items-center gap-1 hover:bg-[var(--bg-light)] cursor-pointer', | ||
active ? 'bg-[var(--bg-light)] border-t border-l border-r' : 'bg-bg-3000', | ||
onClear ? 'pl-3 pr-2' : 'px-3' | ||
)} | ||
> | ||
{model.view?.name ?? 'New query'} | ||
{isEditing ? ( | ||
<AutoTab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: the editing mode is very blended with the existing styling. Not sure exactly best styling here but something slightly to indicate you're in editing mode besides just a blinking cursor would be helpful
Problem
We could not rename tabs in our sql editor previously, and now we can!
Changes
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Renaming tabs works and saves across browser refreshes... Serializing and deserizing work well out of local storage.