-
Notifications
You must be signed in to change notification settings - Fork 30
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: Refactor console objects menu #2013
feat: Refactor console objects menu #2013
Conversation
mofojed
commented
May 14, 2024
- 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
<DropdownMenu | ||
actions={actions} | ||
onMenuOpened={() => searchRef.current?.focus()} | ||
onMenuClosed={() => setFilterText('')} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- 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
Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>
b235d4d
to
fd0b236
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2013 +/- ##
==========================================
- Coverage 46.66% 46.65% -0.02%
==========================================
Files 692 692
Lines 38629 38576 -53
Branches 9814 9819 +5
==========================================
- Hits 18028 17996 -32
+ Misses 20548 20527 -21
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
onMenuClosed={() => { | ||
searchRef.current?.focus(); | ||
}} |
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.
Why focus on close?
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.
I was being dumb. I select the text on menu open so you can just start typing, and also reset if the menu is closed for more than 5s.
Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>
- Add some comments - Reset text in search field after a delay - Select the text on menu open so you can just start typing