chore(sidebar): sidebar editor database #33808
Conversation
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: 0 B Total Size: 2.57 MB ℹ️ View Unchanged
|
…into sidebar-editor-database
Co-authored-by: Marius Andra <marius.andra@gmail.com>
There was a problem hiding this comment.
PR Summary
Major refactoring of the SQL editor's sidebar UI to reduce layout clutter by consolidating multiple sidebars into a unified tree structure.
- Introduced new
DatabaseTreecomponent infrontend/src/layout/panel-layout/DatabaseTree/DatabaseTree.tsxto integrate with existing panel layout system - Refactored
QueryWindowandQueryPanefrom column-based to row-based flexbox to support the new panel layout - Added
QuerySecondaryPanelenum inmultitabEditorLogic.tsxto manage Variables and Info panels visibility - Integrated editor sidebar functionality with panel layout system through
PanelLayoutNavBarDatabase navigation item - Removed separate
EditorSidebarwhile preserving all functionality through the tree sidebar structure
12 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
| { | ||
| identifier: 'Database', | ||
| id: 'Database', | ||
| icon: <IconDatabase />, | ||
| onClick: (e?: React.KeyboardEvent) => { | ||
| if (!e || e.key === 'Enter' || e.key === ' ' || e.key === 'ArrowRight') { | ||
| handlePanelTriggerClick('Database') | ||
| } | ||
| }, | ||
| showChevron: true, | ||
| }, |
There was a problem hiding this comment.
logic: Both Database and Data management items use the same <IconDatabase /> icon, which could be confusing to users. Consider using distinct icons to differentiate these sections.
| {activePanelIdentifier === 'Shortcuts' && ( | ||
| <ProjectTree root="shortcuts://" searchPlaceholder="Search your shortcuts" /> | ||
| )} | ||
| {activePanelIdentifier === 'Database' && <DatabaseTree />} |
There was a problem hiding this comment.
style: consider adding a searchPlaceholder prop to DatabaseTree for consistency with other tree components
| => | ||
| setSecondaryPanel( | ||
| secondaryPanel === QuerySecondaryPanel.Variables ? null : QuerySecondaryPanel.Variables | ||
| ) | ||
| } |
There was a problem hiding this comment.
style: This pattern is repeated twice. Consider extracting panel toggle logic into a reusable function to avoid duplication
| {secondaryPanel && ( | ||
| <div className="relative flex flex-row w-1/2"> | ||
| {secondaryPanel === QuerySecondaryPanel.Info && ( | ||
| <QueryInfo codeEditorKey={props.codeEditorProps.queryKey} /> |
There was a problem hiding this comment.
logic: codeEditorProps.queryKey might be undefined since it's part of Partial<CodeEditorProps>. Add null check to prevent runtime errors.
|
need a new icon for database sidebar item |
|
Hmmm, can't say I'm a fan of the right side panel. I think having materialisation and variables back in the output pane (but with icons on the tabs) would be better. I also worry that someone may close the database panel and not realise how to reopen it (finding "database" in a side panel full of other items isn't obvious). Maybe adding a icon-only "database" button to the left of the query tabs to open the panel would be helpful here |
📸 UI snapshots have been updated16 snapshot changes in total. 0 added, 16 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
If I have no views, the view looks weird:
I really want to just click the table and get a query. Could we have either as the first item when the table opens a "--> select *" button that makes a query with the table, or then add it to the dropdown here:
Basically I'm missing some way to go to the query editor (or just some kind of data browser) from the database view.
Then +1 to the idea of putting a "toggle database" or similar button to the left of the tabs in the query editor, which would show/hide the panel.
I'm not against the right tabs, though it feels like they could be full height perhaps? They're not perfect as they are, but overall with the sidebars I'd say this is all a step in the right direction, so... 🤔
|
Some more state cleanup but worth a look with these changes:
|
|
I think we could have a better default for materialization where we say "Save and materialize" or hide the tab all together although I think the former is better. I am happy with a more landscape space to work with as we add some more elements here, like longer tables and a graph for example. |
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
good point, bringing this back |
…into sidebar-editor-database
| urlToAction({ | ||
| [urls.sqlEditor()]: () => { | ||
| panelLayoutLogic.actions.setActivePanelIdentifier('Database') | ||
| }, | ||
| }), |
There was a problem hiding this comment.
I didn't check, but this may cause some issues if you have a very small screen --> the panel might open in a weird way on top of everything else
| <DropdownMenuItem | ||
| asChild | ||
| => { | ||
| e.stopPropagation() | ||
| selectSourceTable(item.name) | ||
| if (router.values.location.pathname.endsWith(urls.sqlEditor())) { |
There was a problem hiding this comment.
This might be nicer as a selector somewhere
Problem
Changes
TODO: