Skip to content

Conversation

@kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Nov 6, 2024

Description

Merging this PR will:

  • Add a connect action, represented as a Button to every disconnected connection item in the sidebar.
  • Disable the default action (which was connecting) when clicking the connection in the sidebar as well as removing the visual cue for the interaction (the cursor: 'pointer').
  • Update E2E tests to connect by clicking the new button.
  • Invert a test which expected clicking an inactive connection to connect.
connect-button.mov

Checklist

  • Update e2e tests to click the connect button
  • Fix the breaking @mongodb-js/compass-sidebar tests
  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

  • Given the intent to eventually extend this "Connect" button with a drop-down, I wonder if the existing ItemAction abstraction is the right for this feature or I should introduce another prop to inject a custom component into the NavigationBaseItem.

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@kraenhansen kraenhansen self-assigned this Nov 6, 2024
@github-actions github-actions bot added the feat label Nov 6, 2024
@kraenhansen kraenhansen changed the title feat(connections-navigation): add connect button to connections in sidebar COMPASS-8381 chore(connections-navigation): add connect button to connections in sidebar COMPASS-8381 Nov 6, 2024
@kraenhansen kraenhansen marked this pull request as ready for review November 8, 2024 17:01
isFocused={isFocused}
isExpanded={!!item.isExpanded}
hasDefaultAction={
item.type !== 'connection' || item.connectionStatus === 'connected'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this would be co-located with or derived from the implementation in onDefaultAction in connections-navigation-tree.tsx, but that is not trivial because of the virtualization.

Copy link
Contributor

@himanshusinghs himanshusinghs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Something else I noticed, definitely not urgent for now. I feel like the ItemActionControls component interface need a little revisit (maybe from design) because one combination will render the UI un-usable, having collapseAfter set to anything >1 and one or more of the rendered actions having expandedPresentation of button. I think what I am trying to suggest is having some typecheck (if possible) to only allow combination that make sense from rendering POV but yea that depends on what design is thinks is feasible from rendering POV.

@himanshusinghs
Copy link
Contributor

himanshusinghs commented Nov 12, 2024

I ran the branch locally and have two clarifying questions:
1. I suppose we don't even render the connections in the disconnected mode in MMS so Betsy's concern about not showing the button on compass-web should be addressed by that but can you please confirm this? (Incorrectly interpreted the comment in the ticket so please ignore 😃)
2. Do we want to cross-confirm with design whether the right-arrow icon still make sense when disconnected? It definitely made sense earlier as it signified that the rows could be expanded (even when disconnected), maybe not so much anymore?

@gribnoysup
Copy link
Collaborator

I suppose we don't even render the connections in the disconnected mode

We do, we don't automatically connect to every cluster in the list

Copy link
Contributor

@betsybutton betsybutton left a comment

Choose a reason for hiding this comment

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

LGTM from a UX perspective!

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 12, 2024

Do we want to cross-confirm with design whether the right-arrow icon still make sense when disconnected? It definitely made sense earlier as it signified that the rows could be expanded (even when disconnected), maybe not so much anymore?

I think that's a valid point.

  • if removed completely, it will introduce some "jumping" of elements as the connection gets established
  • if left blank, it will introduce some strange whitespace

Perhaps a "Disconnect" icon would be more appropriate? As this is out of scope for the original design, I suggest leaving that up for another (quick) PR to avoid holding back this one.

@bsradcliffe what do you think?

@kraenhansen kraenhansen merged commit f0e5322 into main Nov 12, 2024
31 checks passed
@kraenhansen kraenhansen deleted the kh/connect-button branch November 12, 2024 18:33
@bsradcliffe
Copy link
Contributor

I would like to preserve the chevrons. They indicate that a node can be expanded, but it's simply disabled in the collapsed state. This is a fairly standard convention in explorer trees in GUIs. I wouldn't want to introduce any inconsistent leading edges/jagged jumping between the root level nodes.

Disconnect is something that is needs to be "buried" for lack of a better word. It shouldn't be immediately accessible at the top level since a potential misclick can terminate the connection and close all of the associated connection's tabs.

@kraenhansen
Copy link
Contributor Author

Disconnect is something that is needs to be "buried" for lack of a better word. It shouldn't be immediately accessible at the top level since a potential misclick can terminate the connection and close all of the associated connection's tabs.

If this is referring to my last suggestion, I fear I didn't make myself clear:
To clarify, my suggestion was to simply replace the "CaretRight" with the "Disconnect" icon from the Leafygreen icons, for the already disconnected connections in the sidebar. I.e. I'm not suggesting adding any new interaction (which could be misclicked).

@bsradcliffe
Copy link
Contributor

Gotcha. Even so, as we only utilize that left glyph for chevrons, I would still stay no to the proposal. Every left-most glyph should be a caret of some sort where child nodes can exist beneath it.

@bsradcliffe
Copy link
Contributor

Question here: in the video posted, why is it a single connect CTA and not the split button as the designs show?

CleanShot 2024-11-13 at 09 18 07@2x

@betsybutton
Copy link
Contributor

@bsradcliffe We split up the engineering work into 2 tickets, since opening a connection in a new window is a non-trivial task. Here's the other ticket: https://jira.mongodb.org/browse/COMPASS-8473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants