Skip to content
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.

Sign up for GitHub

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(sidebar): connections filter popover COMPASS-8503 #6486

Merged
merged 12 commits into from
Nov 14, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
754ab49
Refactor ConnectionsNavigation into NavigationItemsFilter
kraenhansen Nov 13, 2024
4fd49fc
Refactor to a popover
kraenhansen Nov 13, 2024
b0aea1a
Delete unused icons
kraenhansen Nov 13, 2024
464e79d
Add activated indicator
kraenhansen Nov 13, 2024
6d19bbe
Blur trigger when closing popover
kraenhansen Nov 13, 2024
5e901dd
Fix failing tests
kraenhansen Nov 13, 2024
207bbc1
Avoid className prop on NavigationItemsFilter
kraenhansen Nov 13, 2024
efc191c
fixup! Refactor to a popover
kraenhansen Nov 13, 2024
d02f274
fixup! Blur trigger when closing popover
kraenhansen Nov 13, 2024
03f93b8
Remove comments
kraenhansen Nov 13, 2024
b8f783f
Revert "Blur trigger when closing popover"
kraenhansen Nov 14, 2024
65beec5
Control tooltip to avoid it showing when closing popover
kraenhansen Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 0 additions & 46 deletions packages/compass-components/src/components/icons/connected-plugs.tsx
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions packages/compass-components/src/components/icons/disconnected-plug.tsx
View file
Open in desktop

This file was deleted.

10 changes: 8 additions & 2 deletions packages/compass-components/src/components/interactive-popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type InteractivePopoverProps = {
containedElements?: string[];
containerClassName?: string;
closeButtonClassName?: string;
blurTriggerOnClose?: boolean;
} & Pick<
React.ComponentProps<typeof Popover>,
'align' | 'justify' | 'spacing' | 'popoverZIndex'
Expand All @@ -80,6 +81,7 @@ function InteractivePopover({
popoverZIndex,
containerClassName,
closeButtonClassName,
blurTriggerOnClose = false,
}: InteractivePopoverProps): React.ReactElement {
const darkMode = useDarkMode();
const triggerRef = useRef<HTMLButtonElement>(null);
Expand All @@ -91,9 +93,13 @@ function InteractivePopover({

// Return focus to the trigger when the popover is hidden.
setTimeout(() => {
triggerRef.current?.focus();
if (blurTriggerOnClose) {
triggerRef.current?.blur();
} else {
triggerRef.current?.focus();
}
});
}, [setOpen]);
}, [setOpen, blurTriggerOnClose]);

const onClickTrigger = useCallback(
(event) => {
Expand Down
2 changes: 0 additions & 2 deletions packages/compass-components/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ export { DocumentIcon } from './components/icons/document-icon';
export { FavoriteIcon } from './components/icons/favorite-icon';
export { ServerIcon } from './components/icons/server-icon';
export { NoSavedItemsIcon } from './components/icons/no-saved-items-icon';
export { ConnectedPlugsIcon } from './components/icons/connected-plugs';
export { DisconnectedPlugIcon } from './components/icons/disconnected-plug';
export { GuideCue as LGGuideCue } from '@leafygreen-ui/guide-cue';
export { Variant as BadgeVariant } from '@leafygreen-ui/badge';
export { Variant as BannerVariant } from '@leafygreen-ui/banner';
Expand Down
133 changes: 133 additions & 0 deletions packages/compass-sidebar/src/components/connections-filter-popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import React, { useCallback, type PropsWithChildren } from 'react';

import {
css,
Icon,
IconButton,
InteractivePopover,
Label,
Overline,
palette,
spacing,
Toggle,
Tooltip,
useId,
} from '@mongodb-js/compass-components';
import type { ConnectionsFilter } from './use-filtered-connections';

const containerStyles = css({
display: 'flex',
flexDirection: 'column',
alignItems: 'flex-start',
padding: spacing[300],
minWidth: 270,
});

const closeButtonStyles = css({
// An alternative to this is to pass hideCloseButton to InteractivePopover,
// but that throws an error when the popover is opened
kraenhansen marked this conversation as resolved.
Show resolved Hide resolved
display: 'none',
});

const activatedIndicatorStyles = css({
position: 'absolute',
top: spacing[50],
right: spacing[50],
});

const groupStyles = css({
display: 'flex',
flexDirection: 'row',
gap: spacing[200],
marginTop: spacing[200],
});

type ConnectionsFilterPopoverProps = PropsWithChildren<{
open: boolean;
setOpen: (open: boolean) => void;
filter: ConnectionsFilter;
onFilterChange(
updater: (filter: ConnectionsFilter) => ConnectionsFilter
): void;
}>;

export default function ConnectionsFilterPopover({
open,
setOpen,
filter,
onFilterChange,
}: ConnectionsFilterPopoverProps) {
const onExcludeInactiveChange = useCallback(
(excludeInactive: boolean) => {
onFilterChange((filter) => ({
...filter,
excludeInactive,
}));
},
[onFilterChange]
);

const excludeInactiveToggleId = useId('exclude-inactive-toggle');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const excludeInactiveLabelId = useId('exclude-inactive-label');

// Add future filters to the boolean below
const isActivated = filter.excludeInactive;

return (
<InteractivePopover
open={open}
setOpen={setOpen}
blurTriggerOnClose
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kraenhansen kraenhansen Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 👍

closeButtonClassName={closeButtonStyles}
containerClassName={containerStyles}
trigger={({ onClick, children, ref }) => (
<>
<Tooltip
align="right"
/* enabled={!open} */
kraenhansen marked this conversation as resolved.
Show resolved Hide resolved
trigger={
<IconButton
onClick={onClick}
active={open}
aria-label="Filter connections"
ref={ref as React.Ref<unknown>}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@gribnoysup gribnoysup Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kraenhansen kraenhansen Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

>
{/* TODO: Show a small blue circle when filter.excludeInactive is enabled */}
kraenhansen marked this conversation as resolved.
Show resolved Hide resolved
<Icon glyph="Filter" />
{isActivated && (
<svg
className={activatedIndicatorStyles}
width="6"
height="6"
viewBox="0 0 6 6"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<circle cx="3" cy="3" r="3" fill={palette.blue.base} />
</svg>
)}
</IconButton>
}
>
Filter connections
</Tooltip>
{children}
</>
)}
>
<Overline>Filter Options</Overline>
<div className={groupStyles}>
<Toggle
id={excludeInactiveToggleId}
aria-labelledby={excludeInactiveLabelId}
checked={filter.excludeInactive}
onChange={onExcludeInactiveChange}
size="small"
/>
<Label htmlFor={excludeInactiveToggleId} id={excludeInactiveLabelId}>
Show only active connections
</Label>
</div>
</InteractivePopover>
);
}
73 changes: 15 additions & 58 deletions packages/compass-sidebar/src/components/multiple-connections/connections-navigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ import {
Button,
Icon,
ButtonVariant,
IconButton,
ConnectedPlugsIcon,
DisconnectedPlugIcon,
Tooltip,
} from '@mongodb-js/compass-components';
import { ConnectionsNavigationTree } from '@mongodb-js/compass-connections-navigation';
import type { MapDispatchToProps, MapStateToProps } from 'react-redux';
Expand Down Expand Up @@ -46,7 +42,10 @@ import {
fetchAllCollections,
type Database,
} from '../../modules/databases';
import { useFilteredConnections } from '../use-filtered-connections';
import {
type ConnectionsFilter,
useFilteredConnections,
} from '../use-filtered-connections';
import NavigationItemsFilter from '../navigation-items-filter';
import {
type ConnectionImportExportAction,
Expand Down Expand Up @@ -84,19 +83,6 @@ const connectionCountStyles = css({
marginLeft: spacing[100],
});

const filterContainerStyles = css({
display: 'flex',
flexDirection: 'row',
alignItems: 'center',
gap: spacing[200],
paddingLeft: spacing[400],
paddingRight: spacing[400],
});

const searchFormStyles = css({
flexGrow: 1,
});

const noDeploymentStyles = css({
paddingLeft: spacing[400],
paddingRight: spacing[400],
Expand All @@ -123,10 +109,10 @@ type ConnectionListTitleActions =
type ConnectionsNavigationComponentProps = {
connectionsWithStatus: ReturnType<typeof useConnectionsWithStatus>;
activeWorkspace: WorkspaceTab | null;
filterRegex: RegExp | null;
excludeInactive: boolean;
onFilterChange(regex: RegExp | null): void;
onToggleExcludeInactive(): void;
filter: ConnectionsFilter;
Copy link
Contributor Author

@kraenhansen kraenhansen Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

onFilterChange(
updater: (filter: ConnectionsFilter) => ConnectionsFilter
): void;
onConnect(info: ConnectionInfo): void;
onNewConnection(): void;
onEditConnection(info: ConnectionInfo): void;
Expand Down Expand Up @@ -167,13 +153,11 @@ type ConnectionsNavigationProps = ConnectionsNavigationComponentProps &
const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
connectionsWithStatus,
activeWorkspace,
filterRegex,
excludeInactive,
filter,
instances,
databases,
isPerformanceTabSupported,
onFilterChange,
onToggleExcludeInactive,
onConnect,
onNewConnection,
onEditConnection,
Expand Down Expand Up @@ -270,10 +254,9 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
onDatabaseToggle,
} = useFilteredConnections({
connections,
filterRegex,
filter,
fetchAllCollections,
onDatabaseExpand,
excludeInactive,
});

const connectionListTitleActions =
Expand Down Expand Up @@ -519,37 +502,11 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
</div>
{connections.length > 0 && (
<>
<div className={filterContainerStyles}>
<NavigationItemsFilter
className={searchFormStyles}
placeholder="Search connections"
onFilterChange={onFilterChange}
/>
<Tooltip
justify="middle"
trigger={
<IconButton
onClick={onToggleExcludeInactive}
active={excludeInactive}
aria-label={
excludeInactive
? 'Showing active connections'
: 'Showing all connections'
}
>
{excludeInactive ? (
<ConnectedPlugsIcon />
) : (
<DisconnectedPlugIcon />
)}
</IconButton>
}
>
{excludeInactive
? 'Showing active connections'
: 'Showing all connections'}
</Tooltip>
</div>
<NavigationItemsFilter
placeholder="Search connections"
filter={filter}
onFilterChange={onFilterChange}
/>
<ConnectionsNavigationTree
connections={filtered || connections}
activeWorkspace={activeWorkspace}
Expand Down
11 changes: 4 additions & 7 deletions packages/compass-sidebar/src/components/multiple-connections/sidebar.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,16 +363,13 @@ describe('Multiple Connections Sidebar Component', function () {
const favoriteConnectionId = savedFavoriteConnection.id;
const recentConnectionId = savedRecentConnection.id;

const activeConnectionsToggleButton = screen.getByLabelText(
'Showing all connections'
);

expect(screen.queryByTestId(favoriteConnectionId)).to.be.visible;
expect(screen.queryByTestId(recentConnectionId)).to.be.visible;

userEvent.click(activeConnectionsToggleButton);
expect(activeConnectionsToggleButton.ariaLabel).equals(
'Showing active connections'
userEvent.click(screen.getByLabelText('Filter connections'));

userEvent.click(
screen.getByLabelText('Show only active connections')
);

expect(screen.queryByTestId(favoriteConnectionId)).to.be.null;
Expand Down
Loading
Loading