Skip to content

feat: Refactor console objects menu#2013

Merged
mofojed merged 6 commits intodeephaven:mainfrom
mofojed:1884-console-hide-objects-menu
Aug 14, 2024
Merged

feat: Refactor console objects menu#2013
mofojed merged 6 commits intodeephaven:mainfrom
mofojed:1884-console-hide-objects-menu

Conversation

@mofojed
Copy link
Copy Markdown
Member

@mofojed 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

@mofojed mofojed requested a review from mattrunyon May 14, 2024 17:57
@mofojed mofojed self-assigned this May 14, 2024
Comment thread packages/console/src/Console.tsx Outdated
Comment thread packages/console/src/ConsoleObjectsMenu.tsx Outdated
Comment thread packages/console/src/ConsoleObjectsMenu.tsx Outdated
Comment thread packages/console/src/ConsoleObjectsMenu.tsx Outdated
<DropdownMenu
actions={actions}
onMenuOpened={() => searchRef.current?.focus()}
onMenuClosed={() => setFilterText('')}
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member Author

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.

Comment thread packages/console/src/ConsoleStatusBar.tsx
Comment on lines +69 to +78
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';
}
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

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.

Comment thread packages/console/src/ConsoleStatusBar.tsx Outdated
mofojed and others added 3 commits August 6, 2024 13:02
- 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>
@mofojed mofojed force-pushed the 1884-console-hide-objects-menu branch from b235d4d to fd0b236 Compare August 6, 2024 19:36
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 56.89655% with 25 lines in your changes missing coverage. Please review.

Project coverage is 46.65%. Comparing base (a257296) to head (7ef71b8).
Report is 2 commits behind head on main.

Files Patch % Lines
packages/console/src/ConsoleObjectsMenu.tsx 42.85% 16 Missing ⚠️
packages/console/src/ConsoleStatusBar.tsx 72.00% 7 Missing ⚠️
packages/components/src/SearchInput.tsx 0.00% 2 Missing ⚠️
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              
Flag Coverage Δ
unit 46.65% <56.89%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mofojed mofojed requested a review from mattrunyon August 6, 2024 19:52
Comment thread packages/console/src/Console.tsx
Comment thread packages/console/src/ConsoleObjectsMenu.tsx
Comment on lines +80 to +82
onMenuClosed={() => {
searchRef.current?.focus();
}}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why focus on close?

Copy link
Copy Markdown
Member Author

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.

Comment thread packages/console/src/ConsoleStatusBar.tsx Outdated
Comment thread packages/console/src/ConsoleStatusBar.tsx
mofojed and others added 2 commits August 13, 2024 08:52
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
@mofojed mofojed requested a review from mattrunyon August 13, 2024 13:36
Comment thread packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx Outdated
@mofojed mofojed merged commit 8251180 into deephaven:main Aug 14, 2024
@mofojed mofojed deleted the 1884-console-hide-objects-menu branch August 14, 2024 16:47
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deephaven.ui widgets do not appear in Console dropdowns

2 participants