feat: Refactor console objects menu #2013
Conversation
mofojed
commented
May 14, 2024
mofojed
commented
- Split ConsoleMenu up so objects is it's own component ConsoleObjectsMenu
- Convert to functional component
- Display all widgets in the menu
- Add a flag to hide the menu, which we set for DHC, but will not be set in DHE so current behaviour is not broken there
- Fixes deephaven.ui widgets do not appear in Console dropdowns #1884
- Split ConsoleMenu up so objects is it's own component ConsoleObjectsMenu - Convert to functional component - Display all widgets in the menu - Add a flag to hide the menu, which we set for DHC, but will not be set in DHE so current behaviour is not broken there
| /** Hide the objects menu in the status bar. Defaults to false. */ | ||
| hideObjectsMenu?: boolean; |
There was a problem hiding this comment.
I think I'd prefer showObjectsMenu defaulting to true to avoid the double negative. Also I think this should this have a defaultProps value?
| onClick={() => { | ||
| // no-op: click is handled in `DropdownMenu` | ||
| }} |
| }; | ||
| const filteredObjects = filterText | ||
| ? objects.filter( | ||
| ({ title }: { title?: string }) => |
There was a problem hiding this comment.
Don't think you need to specify the type here. The JSAPI types say it's always defined and the source looks to confirm that
| <DropdownMenu | ||
| actions={actions} | ||
| onMenuOpened={() => searchRef.current?.focus()} | ||
| onMenuClosed={() => setFilterText('')} |
There was a problem hiding this comment.
I wonder if we should leave the filter and just have the search select the search text when it's reopened. I guess if this is the behavior it has always been and nobody has complained then it's inconsequential. I was just thinking if I'm searching maybe I wanted to open several matching the same search or something
There was a problem hiding this comment.
Yeah, could have it on a timer to reset it to blank after a few seconds. I could have sworn we had that logic on the query widget list on Enterprise, but I can't find it.
I'll just have it select text on open, that's how the QueryWidgetList behaves on Enterprise now.
| } catch (error) { | ||
| // No-op, fall through |
There was a problem hiding this comment.
Why the change to the logic here? Previously looks like we did something w/ the error
| if (isCommandRunning) { | ||
| // Connected, Pending | ||
| statusIconClass = 'console-status-icon-pending'; | ||
| tooltipText = 'Worker is busy'; | ||
| } else { | ||
| // Connected, Idle | ||
| statusIconClass = 'console-status-icon-idle'; | ||
| tooltipText = 'Worker is idle'; | ||
| } |
There was a problem hiding this comment.
No disconnected state after the refactor? This component is consumed by DHE, but I'm not sure how to test/get the disconnected state there.
There was a problem hiding this comment.
Ah crap that's right. That's part of the reason I did the whole XComponent thing in the first place... I considered also porting over ConsoleContainer and all the disconnect/reconnect logic, then just having DHE replace the ConsoleCreator with it's own component... I think I might just do that, seems like it'd be more robust.
There was a problem hiding this comment.
Actually nevermind, isDisconnected isn't used at all in this component. It's always false, and it never gets set anywhere. That's why I removed it, it was never used.
| onClick={() => { | ||
| // no-op: click is handled in `DropdownMenu` | ||
| }} |
Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>
