feat(sidebar): connections filter popover COMPASS-8503 #6486
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| 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 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(); | ||
| const excludeInactiveLabelId = useId(); | ||
|
|
||
| // Add future filters to the boolean below | ||
| const isActivated = filter.excludeInactive; | ||
|
|
||
| return ( | ||
| <InteractivePopover | ||
| open={open} | ||
| setOpen={setOpen} | ||
| blurTriggerOnClose | ||
| containerClassName={containerStyles} | ||
| hideCloseButton | ||
| trigger={({ onClick, children, ref }) => ( | ||
| <> | ||
| <Tooltip | ||
| align="right" | ||
| trigger={ | ||
| <IconButton | ||
| onClick={onClick} | ||
| active={open} | ||
| aria-label="Filter connections" | ||
| ref={ref as React.Ref<unknown>} | ||
|
There was a problem hiding this comment. To be honest, I didn't fully understand the need for this. It seems the There was a problem hiding this comment. InteractivePopover was initially built to be used with There was a problem hiding this comment. I agree, that could probably be fixed with updates to the |
||
| > | ||
| <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> | ||
| ); | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I agree we should avoid regressing further on this 👍
I reverted the change to add a
blurTriggerOnCloseprop and instead usedTooltipas 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 👍