fix: expanding an inactive connection triggers connect COMPASS-8060

[paula-stacho]

Description

Expanding a disconnected item is not possible without connecting first. Then, it'll be expanded by default.
Handling this at the sidebar level, so that the NavigationItem doesn't need to know about this connection specific case.

Checklist

• 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

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)

[himanshusinghs]

🚀

[gribnoysup]

Approved, but I think we are handling this on a wrong level, that's not how disabled elements usually behave

[gribnoysup]

I gotta say that it's a bit of a weird place to handle this. If navigation item is not expandable for whatever reason, it should not be calling onItemExpand, similar to disabled buttons not emitting click events

[paula-stacho]

I totally agree it's weird. But we made a design decision to show the expand button even though this item is technically not expandable until we connect. So we either have the navigation item supporting a "fake expand". Or, as another way to look at it - this item is "expandable by connecting". Which is really what happens - it will be expanded after the connect is done.

[paula-stacho]

Updated the comment to make the intent more clear

[gribnoysup]

I understand the design decision, but I don't see how this affects the technical implementation: currently connection doesn't happen when clicking on the chevron because the button is not disabled (even though we expect it to be) and continues to produce click events that prevent event propagation, which prevents the navigation item default action from being triggered. Instead of changing the button expected state for this case (visible, disabled), we are working around this by handling the click as if the item is disabled even when it shouldn't be produced in the first place.

[paula-stacho]

I don't think we expect it to be disabled, for example we don't expect it to look disabled - at least that's how I understand it's presence. It's not that the click on it does nothing, it just does the same thing as clicking on the item.
But we could replace the current isExpandable with something like isExpandable + isExpandBtnVisible or isExpandBtnVisible + isExpandBtnDisabled. However, it looks like disabled btn doesn't normally propagate the click to the parent, so we'd need to hack that. https://jsfiddle.net/hvg54rdx/

[gribnoysup]

I don't think we expect it to be disabled

I don't agree with that, we have a button in this component which role is to communicate expansion action from the user input, not connecting one or opening a workspace, or something else, it's an expand button, it looks like one and the interface that it provides is very explicit about it by the means of calling it onItemExpand. When we are not connected, this button should be there just for visuals, not function, which has it's own ways of being implemented when using html and css: making button disabled, not allowing pointer events with pointer-events: none styles. We can also rename the handler to onItemExpandOrDefaultActionWhenDisabled to make it clear that the handler will not only be triggered when the actual expand action is happening, that's also a way to solve the inconsistency of the interface provided and the actual behavior.

We can avoid using the basic platform apis we are developing for, there are good reasons for that sometimes, but it always results in more code to maintain. In this particular case we now need a comment here explaining why connection dispatching happens in the expand handler and we need to keep in mind that any changes to this logic would now need to be applied in two places instead of one.

[gribnoysup]

To be super clear, we genuinely can solve this in any way we can, I don't want to mix two topics into one which I totally did with my initial comment. Whether or not we use web tools or javascript code in React to make the expected behavior we need is less important here.

I am really not convinved though that this is not the responsibility of navigation-item to handle this, it is already aware of it's disconnected state so that we can apply visual changes for this state. Expanded button only visually being present there, but not actually being an active, functional button to me looks like another requirement for the navigation-item component, not something that should be solved outside of it.