feat(sidebar): connections filter popover COMPASS-8503 #6486

kraenhansen · 2024-11-13T14:51:07Z

Description

Merging this PR will:

Refactor the toggle-able active connections icon-button in the sidebar to a popover with a toggle.
Delete icons that was added but is no longer used.
Add a blue indicator on the icon button when a filter from within the popover is activated.
Update tests to reflect the changes.

connections-filter-popover-2.mov

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)

bsradcliffe · 2024-11-13T15:39:06Z

@kraenhansen this LGTM! 👍🏻

We'll follow up with the LG team on the composability and configurability of their foundational components like labels, overline, and popovers.

bsradcliffe

LGTM! 👍🏻

kraenhansen · 2024-11-13T15:44:58Z

Seems the test are failing as our CI infrastructure is getting rate-limited by the Docker registry.

lerouxb · 2024-11-13T15:45:51Z

packages/compass-sidebar/src/components/use-filtered-connections.ts

@@ -388,16 +388,19 @@ function filteredConnectionsToSidebarConnection(
  return sidebarConnections;
 }

+export type ConnectionsFilter = {
+  regex: RegExp | null;


Nit and not something you changed, but I'm wondering why this ever used explicit null over just being optional / undefined.

It does 👍

compass/packages/compass-sidebar/src/components/navigation-items-filter.tsx

Lines 23 to 27 in 28bb675

try {

re = searchString ? new RegExp(searchString, 'i') : null;

} catch (e) {

re = null;

}

kraenhansen · 2024-11-13T15:46:43Z

packages/compass-sidebar/src/components/multiple-connections/connections-navigation.tsx

-  excludeInactive: boolean;
-  onFilterChange(regex: RegExp | null): void;
-  onToggleExcludeInactive(): void;
+  filter: ConnectionsFilter;


I've combined the filterRegex and excludeInactive into a single filter prop as adding the pop-over suggests we might be adding more parameters for filtering in the future.

Makes sense. I believe that's the plan with the popover.

packages/compass-sidebar/src/components/connections-filter-popover.tsx

gribnoysup · 2024-11-13T15:49:20Z

packages/compass-sidebar/src/components/connections-filter-popover.tsx

+    [onFilterChange]
+  );
+
+  const excludeInactiveToggleId = useId('exclude-inactive-toggle');


useId with default argument just returns the argument you are passing, there is no reason to use it here if you're ids are constant

Looking at use elsewhere in the codebase, I was under the impression that this would add some suffix to avoid conflicts if the component is mounted multiple places in the tree? But apparently I was wrong in that assumption 😬 Thanks.

Yeah, good point, seems like a few misuses slipped through, opene #6491 to fix those

gribnoysup · 2024-11-13T15:52:26Z

packages/compass-sidebar/src/components/connections-filter-popover.tsx

+    <InteractivePopover
+      open={open}
+      setOpen={setOpen}
+      blurTriggerOnClose


It's a common accessible behavior to return focus back to the trigger, especially helpful if you're navigating with a keyboard, all leafygreen Menus are doing that and we are following their behavior here with our InteractivePopover. Why are we breaking the pattern here?

I added this because bringing back the focus makes the tooltip show even if it's not hoved 🤔 Perhaps I can return focus to something else?

For what it's worth, it seems the "tab position" is remembered. I.e. when tabbing after closing the popover you can continue to the connections tree.

Your focus is on body though, you don't get notificed by voice over that you're back on button. If we want to really address this, we probably should make sure that the button has a proper (aria-)label and shows tooltip only on hover where it makes sense to show it, not on keyboard focus, but this probably requires figuring this out with leafygreen team. You can probably circumvent this by using an extra sibling element as trigger and not the IconButton itself, this should also work. Or we can just ignore it, we're compromising on accessibility in a lot of other places too already 🤷

One thing I would prefer though if possible is not to add a prop to a shared component that explicitly breaks its accessibility behavior. If we can hack around this outside of it to make this tooltip not show up in this exact place where we need this for some reason, that would be nice

we're compromising on accessibility in a lot of other places too already 🤷

Yeah, but I agree we should avoid regressing further on this 👍

I reverted the change to add a blurTriggerOnClose prop and instead used Tooltip as a controlled component. I also updated the screen capture in the description above ☝️ Thanks a lot for the explanations and the comment in the first place 👍

Pass hideCloseButton instead of using display none

Fix generating unique ids

This reverts part of commit 6d19bbe.

kraenhansen · 2024-11-14T09:06:38Z

packages/compass-sidebar/src/components/connections-filter-popover.tsx

+                onClick={onClick}
+                active={open}
+                aria-label="Filter connections"
+                ref={ref as React.Ref<unknown>}


To be honest, I didn't fully understand the need for this. It seems the InteractivePopover is using the React.LegacyRef type for ref, but 🤷 it works.

InteractivePopover was initially built to be used with button which has this specific type of the ref. It's our code, so if we need to change this to Ref (can probably change it to be a more generic HTMLElement instead of HTMLButtonElement too, but it might have more of a cascading effect) in the component types, we can, it would make sense and would still be compatible with types of "normal" html button refs

I agree, that could probably be fixed with updates to the InteractivePopover. If you're okay with it, I suggest making that a follow-up PR.

kraenhansen · 2024-11-14T12:05:14Z

I've already had a successful run on CI this morning and I've verified the changes I made since pass tests locally.
The failing jobs on CI is unrelated and as this is marked as a blocker for a new GA release, I'll go ahead and merge it.

lerouxb · 2024-11-14T14:04:25Z

packages/compass-sidebar/src/components/connections-filter-popover.tsx

+  // Add future filters to the boolean below
+  const isActivated = filter.excludeInactive;
+
+  // Manually handling the tooltip state instead of supplying a trigger


lerouxb · 2024-11-14T14:04:41Z

packages/compass-sidebar/src/components/connections-filter-popover.tsx

+
+  // Manually handling the tooltip state instead of supplying a trigger
+  // we do this to avoid the tooltip from rendering when the popover is open
+  // and when the IconButton regains focus as the


Already merged, but your comment ends mid-sentence :)

Thanks: #6502

kraenhansen added 7 commits November 13, 2024 11:43

Refactor ConnectionsNavigation into NavigationItemsFilter

754ab49

Refactor to a popover

4fd49fc

Delete unused icons

b0aea1a

Add activated indicator

464e79d

Blur trigger when closing popover

6d19bbe

Fix failing tests

5e901dd

Avoid className prop on NavigationItemsFilter

207bbc1

kraenhansen added the release notes label Nov 13, 2024

kraenhansen self-assigned this Nov 13, 2024

github-actions bot added the feat label Nov 13, 2024

kraenhansen requested a review from bsradcliffe November 13, 2024 15:37

bsradcliffe approved these changes Nov 13, 2024

View reviewed changes

kraenhansen marked this pull request as ready for review November 13, 2024 15:42

lerouxb reviewed Nov 13, 2024

View reviewed changes

lerouxb approved these changes Nov 13, 2024

View reviewed changes

kraenhansen commented Nov 13, 2024

View reviewed changes

gribnoysup reviewed Nov 13, 2024

View reviewed changes

kraenhansen added 3 commits November 13, 2024 17:59

fixup! Refactor to a popover

efc191c

Pass hideCloseButton instead of using display none

fixup! Blur trigger when closing popover

d02f274

Fix generating unique ids

Remove comments

03f93b8

kraenhansen requested a review from gribnoysup November 13, 2024 19:31

Revert "Blur trigger when closing popover"

b8f783f

This reverts part of commit 6d19bbe.

kraenhansen commented Nov 14, 2024

View reviewed changes

Control tooltip to avoid it showing when closing popover

65beec5

kraenhansen merged commit 5a6c445 into main Nov 14, 2024
20 of 25 checks passed

kraenhansen deleted the kh/connections-filter-popover branch November 14, 2024 12:05

kraenhansen mentioned this pull request Nov 14, 2024

chore(components): refactor InteractivePopover to take a type argument for the trigger element #6495

Merged

11 tasks

lerouxb reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sidebar): connections filter popover COMPASS-8503 #6486

feat(sidebar): connections filter popover COMPASS-8503 #6486

kraenhansen commented Nov 13, 2024 •

edited

Loading

bsradcliffe commented Nov 13, 2024

bsradcliffe left a comment

kraenhansen commented Nov 13, 2024

lerouxb Nov 13, 2024

kraenhansen Nov 13, 2024

kraenhansen Nov 13, 2024 •

edited

Loading

lerouxb Nov 13, 2024

gribnoysup Nov 13, 2024

kraenhansen Nov 13, 2024

gribnoysup Nov 14, 2024

gribnoysup Nov 13, 2024

kraenhansen Nov 13, 2024

kraenhansen Nov 13, 2024

gribnoysup Nov 14, 2024

gribnoysup Nov 14, 2024

kraenhansen Nov 14, 2024 •

edited

Loading

kraenhansen Nov 14, 2024

gribnoysup Nov 14, 2024 •

edited

Loading

kraenhansen Nov 14, 2024 •

edited

Loading

kraenhansen commented Nov 14, 2024 •

edited

Loading

lerouxb Nov 14, 2024

lerouxb Nov 14, 2024

kraenhansen Nov 15, 2024

	try {
	re = searchString ? new RegExp(searchString, 'i') : null;
	} catch (e) {
	re = null;
	}

feat(sidebar): connections filter popover COMPASS-8503 #6486

feat(sidebar): connections filter popover COMPASS-8503 #6486

Conversation

kraenhansen commented Nov 13, 2024 • edited Loading

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

bsradcliffe commented Nov 13, 2024

bsradcliffe left a comment

Choose a reason for hiding this comment

kraenhansen commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

kraenhansen Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

kraenhansen commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen commented Nov 13, 2024 •

edited

Loading

kraenhansen Nov 13, 2024 •

edited

Loading

kraenhansen Nov 14, 2024 •

edited

Loading

gribnoysup Nov 14, 2024 •

edited

Loading

kraenhansen Nov 14, 2024 •

edited

Loading

kraenhansen commented Nov 14, 2024 •

edited

Loading