From a7330012cdb0b5c1ef86aa686f8fb12d5fde3024 Mon Sep 17 00:00:00 2001 From: Christopher DuBois Date: Thu, 12 Dec 2024 15:11:09 -0800 Subject: [PATCH 1/2] fix(select): fix multi select popovers within parent popover --- src/components/Popover/Popover.stories.tsx | 81 ++++++++++++++++ src/components/Popover/Popover.tsx | 3 +- src/components/Popover/Popover.types.ts | 2 + src/components/Popover/Popover.unit.test.tsx | 35 +++++++ src/components/Popover/Popover.utils.test.ts | 97 +++++++++++++++++-- src/components/Popover/Popover.utils.ts | 7 +- .../tippy-plugins/singleOpenPlugin.test.ts | 94 ++++++++++++++++++ .../Popover/tippy-plugins/singleOpenPlugin.ts | 45 +++++++++ src/components/Select/Select.tsx | 2 + src/components/Select/Select.types.ts | 2 + src/components/Select/Select.unit.test.tsx | 40 +++++++- 11 files changed, 398 insertions(+), 10 deletions(-) create mode 100644 src/components/Popover/tippy-plugins/singleOpenPlugin.test.ts create mode 100644 src/components/Popover/tippy-plugins/singleOpenPlugin.ts diff --git a/src/components/Popover/Popover.stories.tsx b/src/components/Popover/Popover.stories.tsx index 3f3805cd22..5da19791fd 100644 --- a/src/components/Popover/Popover.stories.tsx +++ b/src/components/Popover/Popover.stories.tsx @@ -20,6 +20,7 @@ import SearchInput from '../SearchInput'; import List from '../List'; import AriaToolbarItem from '../AriaToolbarItem'; import ListItemBase from '../ListItemBase'; +import Select from '../Select'; export default { title: 'Momentum UI/Popover', @@ -305,6 +306,84 @@ NestedPopover.args = { ), }; +const MultipleSelectPopoversInsidePopoverInteractiveContentManaged = Template( + Popover +).bind({}); + +MultipleSelectPopoversInsidePopoverInteractiveContentManaged.argTypes = { ...argTypes }; + +MultipleSelectPopoversInsidePopoverInteractiveContentManaged.args = { + trigger: 'click', + placement: PLACEMENTS.BOTTOM, + showArrow: true, + interactive: true, + variant: 'small', + color: COLORS.TERTIARY, + delay: [0, 0], + triggerComponent: ( + Click me! + ), + children: ( + <> + + + + ), +}; + +const MultipleSelectPopoversInsidePopoverInteractiveContentUnmanaged = Template( + Popover +).bind({}); + +MultipleSelectPopoversInsidePopoverInteractiveContentUnmanaged.argTypes = { ...argTypes }; + +MultipleSelectPopoversInsidePopoverInteractiveContentUnmanaged.args = { + trigger: 'click', + placement: PLACEMENTS.BOTTOM, + showArrow: true, + interactive: true, + variant: 'small', + color: COLORS.TERTIARY, + delay: [0, 0], + triggerComponent: ( + Click me! + ), + children: ( + <> + + + + ), +}; + const WithMeetingListItem = Template(Popover).bind({}); WithMeetingListItem.argTypes = { ...argTypes }; @@ -591,6 +670,8 @@ export { WithCloseButton, Offset, MultiplePopovers, + MultipleSelectPopoversInsidePopoverInteractiveContentManaged, + MultipleSelectPopoversInsidePopoverInteractiveContentUnmanaged, NestedPopover, AvatarExample, Common, diff --git a/src/components/Popover/Popover.tsx b/src/components/Popover/Popover.tsx index f421c8445f..858fe3d699 100644 --- a/src/components/Popover/Popover.tsx +++ b/src/components/Popover/Popover.tsx @@ -44,6 +44,7 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { boundary = DEFAULTS.BOUNDARY, hideOnEsc = DEFAULTS.HIDE_ON_ESC, hideOnBlur = DEFAULTS.HIDE_ON_BLUR, + singleOpenGroupId = undefined, isChildPopoverOpen = DEFAULTS.IS_CHILD_POPOVER_OPEN, addBackdrop = DEFAULTS.ADD_BACKDROP, focusBackOnTrigger: focusBackOnTriggerFromProps, @@ -239,7 +240,7 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { }} animation={false} delay={delay} - plugins={addTippyPlugins(hideOnEsc, hideOnBlur, addBackdrop)} + plugins={addTippyPlugins(hideOnEsc, hideOnBlur, addBackdrop, singleOpenGroupId)} // add arrow height to default offset if arrow is shown: offset={[offsetSkidding, showArrow ? ARROW_HEIGHT + offsetDistance : offsetDistance]} {...{ diff --git a/src/components/Popover/Popover.types.ts b/src/components/Popover/Popover.types.ts index 2b4454ac22..98c43a572d 100644 --- a/src/components/Popover/Popover.types.ts +++ b/src/components/Popover/Popover.types.ts @@ -83,6 +83,8 @@ export type PopoverCommonStyleProps = { */ hideOnBlur?: boolean; + singleOpenGroupId?: string; + /** * Manual control of if the Popover contains child elements that may be open, and not nested within the * Popover. it's possible the focus may shift to something that in the DOM is not actually within the Popover, diff --git a/src/components/Popover/Popover.unit.test.tsx b/src/components/Popover/Popover.unit.test.tsx index ffb874e846..05afc1dec8 100644 --- a/src/components/Popover/Popover.unit.test.tsx +++ b/src/components/Popover/Popover.unit.test.tsx @@ -828,6 +828,41 @@ describe('', () => { expect(modalContainer).toBeInTheDocument(); }); + describe('singleOpenGroupId functionality', () => { + it('should only allow one popover to be open within the same group', async () => { + const user = userEvent.setup(); + + render( + <> + Popover 1} + singleOpenGroupId="group1" + > +

Content 1

+ + Popover 2} + singleOpenGroupId="group1" + > +

Content 2

+ + + ); + + // Open first popover + await openPopoverByClickingOnTriggerAndCheckContent(user, /Popover 1/i, /Content 1/i); + + // Ensure first popover is open + expect(screen.queryByText('Content 2')).not.toBeInTheDocument(); + + // Open second popover + await openPopoverByClickingOnTriggerAndCheckContent(user, /Popover 2/i, /Content 2/i); + + // Ensure first popover is closed and second is open + expect(screen.queryByText('Content 1')).not.toBeInTheDocument(); + }); + }); + const nullRef = null; const callbackRef = jest.fn(); const mutableRef = { current: null }; diff --git a/src/components/Popover/Popover.utils.test.ts b/src/components/Popover/Popover.utils.test.ts index 30bfd12fa7..6313b940c6 100644 --- a/src/components/Popover/Popover.utils.test.ts +++ b/src/components/Popover/Popover.utils.test.ts @@ -2,46 +2,131 @@ import { addTippyPlugins } from './Popover.utils'; import { hideOnEscPlugin } from './tippy-plugins/hideOnEscPlugin'; import { addBackdropPlugin } from './tippy-plugins/backdropPlugin'; import { hideOnBlurPlugin } from './tippy-plugins/hideOnBlurPlugin'; +import { getSingleOpenPlugin } from './tippy-plugins/singleOpenPlugin'; describe('addTippyPlugins', () => { it.each([ - { hideOnEsc: false, hideOnBlur: false, addBackdrop: false, expected: [] }, - { hideOnEsc: true, hideOnBlur: false, addBackdrop: false, expected: [hideOnEscPlugin] }, + { + hideOnEsc: false, + hideOnBlur: false, + addBackdrop: false, + singleOpenGroupId: undefined, + expected: [], + }, + { + hideOnEsc: true, + hideOnBlur: false, + addBackdrop: false, + singleOpenGroupId: undefined, + expected: [hideOnEscPlugin], + }, { hideOnEsc: false, hideOnBlur: true, addBackdrop: false, + singleOpenGroupId: undefined, expected: [hideOnBlurPlugin], }, - { hideOnEsc: false, hideOnBlur: false, addBackdrop: true, expected: [addBackdropPlugin] }, + { + hideOnEsc: false, + hideOnBlur: false, + addBackdrop: true, + singleOpenGroupId: undefined, + expected: [addBackdropPlugin], + }, { hideOnEsc: true, hideOnBlur: true, addBackdrop: false, + singleOpenGroupId: undefined, expected: [hideOnEscPlugin, hideOnBlurPlugin], }, { hideOnEsc: true, hideOnBlur: false, addBackdrop: true, + singleOpenGroupId: undefined, expected: [hideOnEscPlugin, addBackdropPlugin], }, { hideOnEsc: false, hideOnBlur: true, addBackdrop: true, + singleOpenGroupId: undefined, expected: [addBackdropPlugin, hideOnBlurPlugin], }, { hideOnEsc: true, hideOnBlur: true, addBackdrop: true, + singleOpenGroupId: undefined, expected: [hideOnEscPlugin, addBackdropPlugin, hideOnBlurPlugin], }, + { + hideOnEsc: false, + hideOnBlur: false, + addBackdrop: false, + singleOpenGroupId: 'group1', + expected: [getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: true, + hideOnBlur: false, + addBackdrop: false, + singleOpenGroupId: 'group1', + expected: [hideOnEscPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: false, + hideOnBlur: true, + addBackdrop: false, + singleOpenGroupId: 'group1', + expected: [hideOnBlurPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: false, + hideOnBlur: false, + addBackdrop: true, + singleOpenGroupId: 'group1', + expected: [addBackdropPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: true, + hideOnBlur: true, + addBackdrop: false, + singleOpenGroupId: 'group1', + expected: [hideOnEscPlugin, hideOnBlurPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: true, + hideOnBlur: false, + addBackdrop: true, + singleOpenGroupId: 'group1', + expected: [hideOnEscPlugin, addBackdropPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: false, + hideOnBlur: true, + addBackdrop: true, + singleOpenGroupId: 'group1', + expected: [addBackdropPlugin, hideOnBlurPlugin, getSingleOpenPlugin('group1')], + }, + { + hideOnEsc: true, + hideOnBlur: true, + addBackdrop: true, + singleOpenGroupId: 'group1', + expected: [ + hideOnEscPlugin, + addBackdropPlugin, + hideOnBlurPlugin, + getSingleOpenPlugin('group1'), + ], + }, ])( - 'should return $expected when hideOnEsc is $hideOnEsc, hideOnBlur is $hideOnBlur, and addBackdrop is $addBackdrop', - ({ hideOnEsc, hideOnBlur, addBackdrop, expected }) => { - const result = addTippyPlugins(hideOnEsc, hideOnBlur, addBackdrop); + 'should return $expected when hideOnEsc is $hideOnEsc, hideOnBlur is $hideOnBlur, addBackdrop is $addBackdrop, and singleOpenGroupId is $singleOpenGroupId', + ({ hideOnEsc, hideOnBlur, addBackdrop, singleOpenGroupId, expected }) => { + const result = addTippyPlugins(hideOnEsc, hideOnBlur, addBackdrop, singleOpenGroupId); expect(result).toEqual(expected); } ); diff --git a/src/components/Popover/Popover.utils.ts b/src/components/Popover/Popover.utils.ts index 1fcc5bd45e..45d19e2f3d 100644 --- a/src/components/Popover/Popover.utils.ts +++ b/src/components/Popover/Popover.utils.ts @@ -2,11 +2,13 @@ import type { Plugin } from 'tippy.js'; import { hideOnEscPlugin } from './tippy-plugins/hideOnEscPlugin'; import { addBackdropPlugin } from './tippy-plugins/backdropPlugin'; import { hideOnBlurPlugin } from './tippy-plugins/hideOnBlurPlugin'; +import { getSingleOpenPlugin } from './tippy-plugins/singleOpenPlugin'; export const addTippyPlugins = ( hideOnEsc: boolean, hideOnBlur: boolean, - addBackdrop: boolean + addBackdrop: boolean, + singleOpenGroupId?: string ): Array => { const plugins = []; if (hideOnEsc) { @@ -18,5 +20,8 @@ export const addTippyPlugins = ( if (hideOnBlur) { plugins.push(hideOnBlurPlugin); } + if (singleOpenGroupId) { + plugins.push(getSingleOpenPlugin(singleOpenGroupId)); + } return plugins; }; diff --git a/src/components/Popover/tippy-plugins/singleOpenPlugin.test.ts b/src/components/Popover/tippy-plugins/singleOpenPlugin.test.ts new file mode 100644 index 0000000000..0bc0b2e7da --- /dev/null +++ b/src/components/Popover/tippy-plugins/singleOpenPlugin.test.ts @@ -0,0 +1,94 @@ +import { getSingleOpenPlugin } from './singleOpenPlugin'; +import { Instance, Props } from 'tippy.js'; + +const createTippyInstance = (): Instance => + ({ + hide: jest.fn(), + show: jest.fn(), + clearDelayTimeouts: jest.fn(), + destroy: jest.fn(), + disable: jest.fn(), + enable: jest.fn(), + setProps: jest.fn(), + setContent: jest.fn(), + popper: document.createElement('div'), + reference: document.createElement('div'), + state: { isVisible: false }, + } as unknown as Instance); + +describe('getSingleOpenPlugin', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return undefined if no groupId is provided', () => { + const plugin = getSingleOpenPlugin(); + expect(plugin).toBeUndefined(); + }); + + it('should return the same plugin for the same groupId', () => { + const groupId = 'test-group'; + const plugin1 = getSingleOpenPlugin(groupId); + const plugin2 = getSingleOpenPlugin(groupId); + + expect(plugin1).toBe(plugin2); + }); + + it('should create a new plugin if groupId is not in the map', () => { + const groupId = 'new-group'; + const plugin = getSingleOpenPlugin(groupId); + + expect(plugin).toBeDefined(); + expect(plugin.name).toBe(`singleOpen-${groupId}`); + }); + + it('should hide other instances within the same group when one is shown', () => { + const groupId = 'group-1'; + const plugin = getSingleOpenPlugin(groupId); + + const tippy1 = createTippyInstance(); + const tippy2 = createTippyInstance(); + + const instancePlugin1 = plugin.fn(tippy1); + const instancePlugin2 = plugin.fn(tippy2); + + instancePlugin1.onShow(tippy1); + instancePlugin2.onShow(tippy2); + + expect(tippy1.hide).toHaveBeenCalledTimes(1); + expect(tippy2.hide).toHaveBeenCalledTimes(0); + }); + + it('should not hide instances from different groups', () => { + const groupId1 = 'group-1'; + const groupId2 = 'group-2'; + + const plugin1 = getSingleOpenPlugin(groupId1); + const plugin2 = getSingleOpenPlugin(groupId2); + + const tippy1 = createTippyInstance(); + const tippy2 = createTippyInstance(); + + const instancePlugin1 = plugin1.fn(tippy1); + const instancePlugin2 = plugin2.fn(tippy2); + + instancePlugin1.onShow(tippy1); + instancePlugin2.onShow(tippy2); + + expect(tippy1.hide).toHaveBeenCalledTimes(0); + expect(tippy2.hide).toHaveBeenCalledTimes(0); + }); + + it('should remove the instance from the set on hide', () => { + const groupId = 'group-3'; + const plugin = getSingleOpenPlugin(groupId); + + const tippy = createTippyInstance(); + const instancePlugin = plugin.fn(tippy); + + instancePlugin.onShow(tippy); + instancePlugin.onHide(tippy); + + expect(tippy.hide).toHaveBeenCalledTimes(0); + }); +}); diff --git a/src/components/Popover/tippy-plugins/singleOpenPlugin.ts b/src/components/Popover/tippy-plugins/singleOpenPlugin.ts new file mode 100644 index 0000000000..a9efede92e --- /dev/null +++ b/src/components/Popover/tippy-plugins/singleOpenPlugin.ts @@ -0,0 +1,45 @@ +import type { Plugin, Instance } from 'tippy.js'; + +const pluginMap = new Map(); + +/* + Before this plugin there is a bug with opening multiple selects inside of a popover, where the PopoverParent renders 2 PopoverChildren and clicking on either PopoverChildren, events get swallowed and the other PopoverChild will not close + this plugin solves this by managing these popovers within a groupID to hide +*/ +export const getSingleOpenPlugin = (groupId?: string): Plugin | undefined => { + if (!groupId) { + return undefined; + } + + if (pluginMap.has(groupId)) { + const plugin = pluginMap.get(groupId); + + return plugin ? plugin : undefined; + } + + const openTippies = new Set(); + + const newPlugin: Plugin = { + name: `singleOpen-${groupId}`, + fn(instance) { + return { + onShow() { + openTippies.forEach((openInstance) => { + if (openInstance !== instance) { + openInstance.hide(); + } + }); + + openTippies.add(instance); + }, + onHide() { + openTippies.delete(instance); + }, + }; + }, + }; + + pluginMap.set(groupId, newPlugin); + + return newPlugin; +}; diff --git a/src/components/Select/Select.tsx b/src/components/Select/Select.tsx index 80024ae4f8..153a9a8cb9 100644 --- a/src/components/Select/Select.tsx +++ b/src/components/Select/Select.tsx @@ -42,6 +42,7 @@ function Select(props: Props, ref: RefObject(); const hasBeenOpened = useRef(false); @@ -219,6 +220,7 @@ function Select(props: Props, ref: RefObject extends AriaSelectProps { * Whether or not this Select should look disabled, but allow focus, etc. */ shallowDisabled?: boolean; + + popoverSingleOpenGroupId?: string; } export type Props = SelectProps & diff --git a/src/components/Select/Select.unit.test.tsx b/src/components/Select/Select.unit.test.tsx index da623decce..bba447c46c 100644 --- a/src/components/Select/Select.unit.test.tsx +++ b/src/components/Select/Select.unit.test.tsx @@ -463,6 +463,44 @@ describe('Select', () => { }); }); + it('should have expected props on Popover with groupId', async () => { + expect.assertions(1); + + const direction = 'top'; + const popoverSingleOpenGroupId = 'test-group'; + + const wrapper = await mountAndWait( + + ); + + expect(wrapper.find(Popover).props()).toEqual({ + interactive: true, + role: null, + showArrow: false, + singleOpenGroupId: 'test-group', + triggerComponent: expect.any(Object), + trigger: 'manual', + setInstance: expect.any(Function), + placement: direction, + onClickOutside: expect.any(Function), + onHide: expect.any(Function), + hideOnEsc: false, + style: { maxHeight: 'none' }, + className: 'md-select-popover', + children: expect.any(Object), + onKeyDown: expect.any(Function), + onKeyUp: undefined, + strategy: 'absolute', + }); + }); + it('should have expected attributes set when listboxWidth is passed in', async () => { expect.assertions(2); @@ -502,8 +540,6 @@ describe('Select', () => { it('should have expected attributes set when shallowDisabled', async () => { expect.assertions(2); - const direction = 'top'; - const wrapper = await mountAndWait(