opensource/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2026-09-21 05:54:10 +08:00
Code Issues Packages Projects Releases Wiki Activity

[MM-68266] Pass through menu props to popout menu item, guard at menu definition to avoid null component blocking keyboard navigation (#36024)

Browse Source 
* [MM-68266] Pass through menu props to popout menu item, guard at menu definition to avoid null component blocking keyboard navigation

* fix tests

* Update webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/sidebar_channel_menu.test.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This commit is contained in:
Devin Binnie
2026-04-13 14:36:17 -04:00
committed by GitHub
co-authored by coderabbitai[bot]
parent e3b2b0a521
commit 2fb38fe71d
7 changed files with 61 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
webapp/channels/src/components/channel_header_menu/channel_header_menu.tsx
+4 -1
View File
@@ -28,6 +28,7 @@ import {getIsChannelBookmarksEnabled} from 'components/channel_bookmarks/utils';
import * as Menu from 'components/menu';
import {Constants} from 'utils/constants';
import {canPopout, isChannelPopoutWindow} from 'utils/popouts/popout_windows';
import type {GlobalState} from 'types/store';
@@ -149,7 +150,9 @@ export default function ChannelHeaderMenu({dmUser, gmMembers, isMobile, archived
horizontal: 'left',
}}
>
<MenuItemOpenInNewWindow channel={channel}/>
{canPopout() && !isChannelPopoutWindow() && (
<MenuItemOpenInNewWindow channel={channel}/>
)}
{isDirect && (
<ChannelDirectMenu
channel={channel}
webapp/channels/src/components/channel_header_menu/menu_items/open_in_new_window.test.tsx
+5 -6
View File
@@ -8,7 +8,7 @@ import type {ChannelType} from '@mattermost/types/channels';
import {WithTestMenuContext} from 'components/menu/menu_context_test';
import {renderWithContext, screen, userEvent} from 'tests/react_testing_utils';
import {isChannelPopoutWindow, popoutChannel} from 'utils/popouts/popout_windows';
import {popoutChannel} from 'utils/popouts/popout_windows';
import {TestHelper} from 'utils/test_helper';
import MenuItemOpenInNewWindow from './open_in_new_window';
@@ -54,21 +54,20 @@ describe('MenuItemOpenInNewWindow', () => {
beforeEach(() => {
jest.clearAllMocks();
jest.mocked(isChannelPopoutWindow).mockReturnValue(false);
});
test('should render nothing when already in a channel popout', () => {
jest.mocked(isChannelPopoutWindow).mockReturnValue(true);
test('should render menu item and separator', () => {
const channel = TestHelper.getChannelMock({type: 'O' as ChannelType, name: 'town-square'});
const {container} = renderWithContext(
renderWithContext(
<WithTestMenuContext>
<MenuItemOpenInNewWindow channel={channel}/>
</WithTestMenuContext>,
baseState,
);
expect(container).toBeEmptyDOMElement();
expect(screen.getByText('Open in new window')).toBeInTheDocument();
expect(screen.getByRole('separator')).toBeInTheDocument();
});
test('should call popoutChannel when clicked', async () => {
webapp/channels/src/components/channel_header_menu/menu_items/open_in_new_window.tsx
+5 -8
View File
@@ -13,19 +13,19 @@ import {getUserIdFromChannelName} from 'mattermost-redux/utils/channel_utils';
import {getPopoutChannelTitle} from 'components/channel_popout/channel_popout';
import * as Menu from 'components/menu';
import PopoutMenuItem from 'components/popout_menu_item';
import PopoutMenuItem, {type PopoutMenuItemProps} from 'components/popout_menu_item';
import {getChannelRoutePathAndIdentifier} from 'utils/channel_utils';
import {Constants} from 'utils/constants';
import {isChannelPopoutWindow, popoutChannel} from 'utils/popouts/popout_windows';
import {popoutChannel} from 'utils/popouts/popout_windows';
import type {GlobalState} from 'types/store';
interface Props {
interface Props extends PopoutMenuItemProps {
channel: Channel;
}
const MenuItemOpenInNewWindow = ({channel}: Props) => {
const MenuItemOpenInNewWindow = ({channel, ...rest}: Props) => {
const intl = useIntl();
const team = useSelector(getCurrentTeam);
const currentUserId = useSelector(getCurrentUserId);
@@ -37,10 +37,6 @@ const MenuItemOpenInNewWindow = ({channel}: Props) => {
return undefined;
});
if (isChannelPopoutWindow()) {
return null;
}
const handleClick = () => {
if (!team) {
return;
@@ -55,6 +51,7 @@ const MenuItemOpenInNewWindow = ({channel}: Props) => {
<PopoutMenuItem
id='channelOpenInNewWindow'
onClick={handleClick}
{...rest}
/>
<Menu.Separator/>
</>
webapp/channels/src/components/popout_menu_item.tsx
+4 -5
View File
@@ -7,15 +7,13 @@ import {FormattedMessage} from 'react-intl';
import {DockWindowIcon} from '@mattermost/compass-icons/components';
import * as Menu from 'components/menu';
import type {Props as MenuItemProps} from 'components/menu/menu_item';
import {canPopout} from 'utils/popouts/popout_windows';
type Props = {
onClick: () => void;
id?: string;
};
export type PopoutMenuItemProps = Omit<MenuItemProps, 'labels' | 'leadingElement'>;
export default function PopoutMenuItem({onClick, id = 'openInNewWindow'}: Props) {
export default function PopoutMenuItem({onClick, id = 'openInNewWindow', ...rest}: PopoutMenuItemProps) {
if (!canPopout()) {
return null;
}
@@ -31,6 +29,7 @@ export default function PopoutMenuItem({onClick, id = 'openInNewWindow'}: Props)
/>
}
onClick={onClick}
{...rest}
/>
);
}
webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap
+20 -20
View File
@@ -108,10 +108,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should match sn
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -449,10 +449,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should match sn
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -790,10 +790,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should match sn
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -1131,10 +1131,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -1404,10 +1404,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -1713,10 +1713,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -2051,10 +2051,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -2392,10 +2392,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -2733,10 +2733,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
@@ -3074,10 +3074,10 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should show cor
tabindex="-1"
>
<li
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
class="MuiButtonBase-root-JDVeC cxEgXn MuiButtonBase-root MuiMenuItem-root MuiMenuItem-gutters Mui-focusVisible MuiMenuItem-root-dXjcMb iLWjgG MuiMenuItem-root MuiMenuItem-gutters sc-grYavY jmUrfe"
id="channelOpenInNewWindow"
role="menuitem"
tabindex="-1"
tabindex="0"
>
<div
class="leading-element"
webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/sidebar_channel_menu.test.tsx
+19
View File
@@ -9,6 +9,7 @@ import {CategoryTypes} from 'mattermost-redux/constants/channel_categories';
import {renderWithContext, screen, userEvent} from 'tests/react_testing_utils';
import Constants from 'utils/constants';
import {canPopout, isChannelPopoutWindow} from 'utils/popouts/popout_windows';
import {TestHelper} from 'utils/test_helper';
import SidebarChannelMenu from './sidebar_channel_menu';
@@ -22,6 +23,12 @@ jest.mock('react-intl', () => ({
}),
}));
jest.mock('utils/popouts/popout_windows', () => ({
...jest.requireActual('utils/popouts/popout_windows'),
canPopout: jest.fn(() => true),
isChannelPopoutWindow: jest.fn(() => false),
}));
describe('components/sidebar/sidebar_channel/sidebar_channel_menu', () => {
const testChannel = TestHelper.getChannelMock();
const testCategory = TestHelper.getCategoryMock();
@@ -260,4 +267,16 @@ describe('components/sidebar/sidebar_channel/sidebar_channel_menu', () => {
await openMenu();
expect(document.body).toMatchSnapshot();
});
test('should not show Open in new window when popout is not available', async () => {
jest.mocked(canPopout).mockReturnValue(false);
jest.mocked(isChannelPopoutWindow).mockReturnValue(false);
renderWithContext(
<SidebarChannelMenu {...baseProps}/>,
);
await openMenu();
expect(screen.queryByRole('menuitem', {name: 'Open in new window'})).not.toBeInTheDocument();
});
});
webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/sidebar_channel_menu.tsx
+4 -1
View File
@@ -22,6 +22,7 @@ import ChannelMoveToSubmenu from 'components/channel_move_to_sub_menu';
import * as Menu from 'components/menu';
import Constants, {ModalIdentifiers} from 'utils/constants';
import {canPopout, isChannelPopoutWindow} from 'utils/popouts/popout_windows';
import {copyToClipboard} from 'utils/utils';
import type {PropsFromRedux, OwnProps} from './index';
@@ -299,7 +300,9 @@ const SidebarChannelMenu = ({
onToggle: onMenuToggle,
}}
>
<MenuItemOpenInNewWindow channel={channel}/>
{canPopout() && !isChannelPopoutWindow() && (
<MenuItemOpenInNewWindow channel={channel}/>
)}
{markAsReadUnreadMenuItem}
{favoriteUnfavoriteMenuItem}
{muteUnmuteChannelMenuItem}