From 81846bb20fbf6ad91b1d7ffa3e2b57360d23c7a6 Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Wed, 30 Apr 2025 10:09:55 -0700 Subject: [PATCH 1/3] fix(dropdown-menu): Use default link behavior --- .../components/core/menuListItem/index.tsx | 23 ++++--- static/app/components/dropdownMenu/index.tsx | 11 +++- static/app/components/dropdownMenu/item.tsx | 62 ++++++++----------- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/static/app/components/core/menuListItem/index.tsx b/static/app/components/core/menuListItem/index.tsx index 7d789238c41e42..68918c35a720a3 100644 --- a/static/app/components/core/menuListItem/index.tsx +++ b/static/app/components/core/menuListItem/index.tsx @@ -2,7 +2,7 @@ import {memo, useId, useRef, useState} from 'react'; import {createPortal} from 'react-dom'; import {usePopper} from 'react-popper'; import isPropValid from '@emotion/is-prop-valid'; -import {type Theme, useTheme} from '@emotion/react'; +import {css, type Theme, useTheme} from '@emotion/react'; import styled from '@emotion/styled'; import {mergeRefs} from '@react-aria/utils'; @@ -123,7 +123,6 @@ function BaseMenuListItem({ return ( p.theme.form[p.size ?? 'md'].fontSize}; &, - &:hover { + &:hover, + &:focus, + &:focus-visible { color: ${getTextColor}; + box-shadow: none; + outline: none; } ${p => p.disabled && `cursor: default;`} @@ -330,13 +333,13 @@ export const InnerWrap = withChonk( ${p => p.isFocused && - ` - z-index: 1; - /* Background to hide the previous item's divider */ - ::before { - background: ${p.theme.backgroundElevated}; - } - `} + css` + z-index: 1; + /* Background to hide the previous item's divider */ + ::before { + background: ${p.theme.backgroundElevated}; + } + `} `, ChonkInnerWrap ); diff --git a/static/app/components/dropdownMenu/index.tsx b/static/app/components/dropdownMenu/index.tsx index 02bb6648d585a4..fc7fea5312f9e8 100644 --- a/static/app/components/dropdownMenu/index.tsx +++ b/static/app/components/dropdownMenu/index.tsx @@ -218,7 +218,16 @@ function DropdownMenu({ ); } - const activeItems = useMemo(() => removeHiddenItems(items), [items]); + const activeItems = useMemo( + () => + removeHiddenItems(items).map(item => { + return { + ...item, + href: item.to ?? item.externalHref, + }; + }), + [items] + ); const defaultDisabledKeys = useMemo(() => getDisabledKeys(activeItems), [activeItems]); function renderMenu() { diff --git a/static/app/components/dropdownMenu/item.tsx b/static/app/components/dropdownMenu/item.tsx index a07ecc18097599..e9e7eb580a14ed 100644 --- a/static/app/components/dropdownMenu/item.tsx +++ b/static/app/components/dropdownMenu/item.tsx @@ -1,20 +1,16 @@ import {Fragment, useContext, useEffect, useRef} from 'react'; import {useHover, useKeyboard} from '@react-aria/interactions'; import {useMenuItem} from '@react-aria/menu'; -import {mergeProps, mergeRefs} from '@react-aria/utils'; +import {mergeProps} from '@react-aria/utils'; import type {TreeState} from '@react-stately/tree'; import type {Node} from '@react-types/shared'; import type {LocationDescriptor} from 'history'; import type {MenuListItemProps} from 'sentry/components/core/menuListItem'; -import { - InnerWrap as MenuListItemInnerWrap, - MenuListItem, -} from 'sentry/components/core/menuListItem'; -import ExternalLink from 'sentry/components/links/externalLink'; +import {MenuListItem} from 'sentry/components/core/menuListItem'; import Link from 'sentry/components/links/link'; import {IconChevron} from 'sentry/icons'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import normalizeUrl from 'sentry/utils/url/normalizeUrl'; import usePrevious from 'sentry/utils/usePrevious'; import {DropdownMenuContext} from './list'; @@ -112,17 +108,17 @@ function DropdownMenuItem({ ref, ...props }: DropdownMenuItemProps) { - const listElementRef = useRef(null); + const innerWrapRef = useRef(null); const isDisabled = state.disabledKeys.has(node.key); const isFocused = state.selectionManager.focusedKey === node.key; const {key, onAction, to, label, isSubmenu, trailingItems, externalHref, ...itemProps} = node.value ?? {}; const {size} = node.props; const {rootOverlayState} = useContext(DropdownMenuContext); - const navigate = useNavigate(); + const isLink = to || externalHref; const actionHandler = () => { - if (to || externalHref) { + if (isLink) { // Close the menu after the click event has bubbled to the link // Only needed on links that do not unmount the menu if (closeOnSelect) { @@ -166,25 +162,6 @@ function DropdownMenuItem({ // Open submenu on arrow right key press const {keyboardProps} = useKeyboard({ onKeyDown: e => { - if (e.key === 'Enter' && (to || externalHref)) { - // If the user is holding down the meta key, we want to dispatch a mouse event - if (e.metaKey || e.ctrlKey || externalHref) { - const mouseEvent = new MouseEvent('click', { - ctrlKey: e.ctrlKey, - metaKey: e.metaKey, - }); - listElementRef.current - ?.querySelector(`${MenuListItemInnerWrap}`) - ?.dispatchEvent(mouseEvent); - return; - } - - if (to) { - navigate(to); - } - return; - } - if (e.key === 'ArrowRight' && isSubmenu) { state.selectionManager.replaceSelection(node.key); return; @@ -203,32 +180,45 @@ function DropdownMenuItem({ onClose?.(); rootOverlayState?.close(); }, - closeOnSelect: to || externalHref ? false : closeOnSelect, + closeOnSelect: isLink ? false : closeOnSelect, isDisabled, }, state, - listElementRef + innerWrapRef ); // Merged menu item props, class names are combined, event handlers chained, // etc. See: https://react-spectrum.adobe.com/react-aria/mergeProps.html - const mergedProps = mergeProps(props, menuItemProps, hoverProps, keyboardProps); + const mergedProps = mergeProps(props, hoverProps, keyboardProps); const itemLabel = node.rendered ?? label; const makeInnerWrapProps = () => { if (to) { - return {as: Link, to}; + return { + ...menuItemProps, + ref: innerWrapRef, + as: Link, + to: normalizeUrl(to), + href: normalizeUrl(to), + }; } if (externalHref) { - return {as: ExternalLink, href: externalHref}; + return { + ...menuItemProps, + ref: innerWrapRef, + as: 'a' as const, + href: externalHref, + target: '_blank', + rel: 'noreferrer noopener', + }; } - return {as: 'div' as const}; + return {...menuItemProps, ref: innerWrapRef, as: 'div' as const}; }; return ( Date: Wed, 30 Apr 2025 11:49:09 -0700 Subject: [PATCH 2/3] Fix tests --- .../components/dropdownMenu/index.spec.tsx | 72 ++++++------------- static/app/components/dropdownMenu/index.tsx | 1 + static/app/components/dropdownMenu/item.tsx | 32 ++++----- .../dashboards/widgetCard/index.spec.tsx | 8 +-- .../widgetCard/issueWidgetCard.spec.tsx | 2 +- static/app/views/discover/queryList.spec.tsx | 24 +++++-- .../app/views/explore/toolbar/index.spec.tsx | 15 ++-- .../issueDetails/groupEventCarousel.spec.tsx | 6 +- .../groupTags/groupTagValues.spec.tsx | 12 +--- .../tagDetailsDrawerContent.spec.tsx | 61 +++++++++------- static/app/views/nav/orgDropdown.spec.tsx | 15 ++-- static/app/views/nav/userDropdown.spec.tsx | 9 ++- 12 files changed, 120 insertions(+), 137 deletions(-) diff --git a/static/app/components/dropdownMenu/index.spec.tsx b/static/app/components/dropdownMenu/index.spec.tsx index 9e2b0301388cdf..7169ccb17ef809 100644 --- a/static/app/components/dropdownMenu/index.spec.tsx +++ b/static/app/components/dropdownMenu/index.spec.tsx @@ -1,5 +1,4 @@ import {Fragment} from 'react'; -import {RouterFixture} from 'sentry-fixture/routerFixture'; import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; @@ -25,10 +24,7 @@ describe('DropdownMenu', function () { }, ]} triggerLabel="This is a Menu" - />, - { - deprecatedRouterMocks: true, - } + /> ); // Open the mneu @@ -71,10 +67,7 @@ describe('DropdownMenu', function () { }, ]} triggerLabel="Menu" - />, - { - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); @@ -95,10 +88,7 @@ describe('DropdownMenu', function () { - , - { - deprecatedRouterMocks: true, - } + ); // Can be dismissed by clicking outside @@ -164,10 +154,7 @@ describe('DropdownMenu', function () { }, ]} triggerLabel="Menu" - />, - { - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); @@ -254,10 +241,7 @@ describe('DropdownMenu', function () { }, ]} triggerLabel="Menu" - />, - { - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); @@ -271,10 +255,7 @@ describe('DropdownMenu', function () { , - { - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); @@ -285,14 +266,13 @@ describe('DropdownMenu', function () { }); it('closes after clicking external link', async function () { + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + render( , - { - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); @@ -300,39 +280,34 @@ describe('DropdownMenu', function () { await waitFor(() => { expect(screen.queryByRole('menuitemradio')).not.toBeInTheDocument(); }); + + // JSDOM throws an error on navigation to random urls + expect(errorSpy).toHaveBeenCalledTimes(1); }); it('navigates to link on enter', async function () { const onAction = jest.fn(); - const router = RouterFixture(); - render( + const {router} = render( , - { - router, - deprecatedRouterMocks: true, - } + /> ); await userEvent.click(screen.getByRole('button', {name: 'Menu'})); await userEvent.keyboard('{ArrowDown}'); await userEvent.keyboard('{Enter}'); await waitFor(() => { - expect(router.push).toHaveBeenCalledWith( - expect.objectContaining({pathname: '/test2'}) - ); + expect(router.location.pathname).toBe('/test2'); }); expect(onAction).toHaveBeenCalledTimes(1); }); it('navigates to link on meta key', async function () { const onAction = jest.fn(); - const router = RouterFixture(); const user = userEvent.setup(); const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); @@ -344,11 +319,7 @@ describe('DropdownMenu', function () { {key: 'item2', label: 'Item Two', to: '/test2', onAction}, ]} triggerLabel="Menu" - />, - { - router, - deprecatedRouterMocks: true, - } + /> ); await user.click(screen.getByRole('button', {name: 'Menu'})); @@ -366,9 +337,10 @@ describe('DropdownMenu', function () { it('navigates to external link enter', async function () { const onAction = jest.fn(); - const router = RouterFixture(); const user = userEvent.setup(); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + render( , - { - router, - deprecatedRouterMocks: true, - } + /> ); await user.click(screen.getByRole('button', {name: 'Menu'})); @@ -393,5 +361,7 @@ describe('DropdownMenu', function () { await user.keyboard('{Enter}'); expect(onAction).toHaveBeenCalledTimes(1); + // JSDOM throws an error on navigation + expect(errorSpy).toHaveBeenCalledTimes(1); }); }); diff --git a/static/app/components/dropdownMenu/index.tsx b/static/app/components/dropdownMenu/index.tsx index fc7fea5312f9e8..a56ac9cd35053b 100644 --- a/static/app/components/dropdownMenu/index.tsx +++ b/static/app/components/dropdownMenu/index.tsx @@ -223,6 +223,7 @@ function DropdownMenu({ removeHiddenItems(items).map(item => { return { ...item, + // react-aria uses the href prop on item state to determine if the item is a link href: item.to ?? item.externalHref, }; }), diff --git a/static/app/components/dropdownMenu/item.tsx b/static/app/components/dropdownMenu/item.tsx index e9e7eb580a14ed..5b624850421803 100644 --- a/static/app/components/dropdownMenu/item.tsx +++ b/static/app/components/dropdownMenu/item.tsx @@ -8,9 +8,9 @@ import type {LocationDescriptor} from 'history'; import type {MenuListItemProps} from 'sentry/components/core/menuListItem'; import {MenuListItem} from 'sentry/components/core/menuListItem'; +import ExternalLink from 'sentry/components/links/externalLink'; import Link from 'sentry/components/links/link'; import {IconChevron} from 'sentry/icons'; -import normalizeUrl from 'sentry/utils/url/normalizeUrl'; import usePrevious from 'sentry/utils/usePrevious'; import {DropdownMenuContext} from './list'; @@ -187,44 +187,41 @@ function DropdownMenuItem({ innerWrapRef ); - // Merged menu item props, class names are combined, event handlers chained, - // etc. See: https://react-spectrum.adobe.com/react-aria/mergeProps.html - const mergedProps = mergeProps(props, hoverProps, keyboardProps); - const itemLabel = node.rendered ?? label; const makeInnerWrapProps = () => { if (to) { return { - ...menuItemProps, - ref: innerWrapRef, as: Link, - to: normalizeUrl(to), - href: normalizeUrl(to), + to, }; } if (externalHref) { return { - ...menuItemProps, - ref: innerWrapRef, - as: 'a' as const, + as: ExternalLink, href: externalHref, - target: '_blank', - rel: 'noreferrer noopener', }; } - return {...menuItemProps, ref: innerWrapRef, as: 'div' as const}; + return {as: 'div' as const}; }; + const mergedMenuItemContentProps = mergeProps( + props, + menuItemProps, + hoverProps, + keyboardProps, + makeInnerWrapProps(), + {ref: innerWrapRef, 'data-test-id': key} + ); + const itemLabel = node.rendered ?? label; return ( ); diff --git a/static/app/views/dashboards/widgetCard/index.spec.tsx b/static/app/views/dashboards/widgetCard/index.spec.tsx index 98f719625bc620..18a18c92650c19 100644 --- a/static/app/views/dashboards/widgetCard/index.spec.tsx +++ b/static/app/views/dashboards/widgetCard/index.spec.tsx @@ -162,7 +162,7 @@ describe('Dashboards > WidgetCard', function () { ); await userEvent.click(await screen.findByLabelText('Widget actions')); - expect(screen.getByRole('link', {name: 'Open in Discover'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Open in Discover'})).toHaveAttribute( 'href', '/organizations/org-slug/discover/results/?environment=prod&field=count%28%29&field=failure_count%28%29&name=Errors&project=1&query=event.type%3Aerror&statsPeriod=14d&yAxis=count%28%29&yAxis=failure_count%28%29' ); @@ -221,7 +221,7 @@ describe('Dashboards > WidgetCard', function () { ); await userEvent.click(await screen.findByLabelText('Widget actions')); - expect(screen.getByRole('link', {name: 'Open in Discover'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Open in Discover'})).toHaveAttribute( 'href', '/organizations/org-slug/discover/results/?environment=prod&field=count_if%28transaction.duration%2Cequals%2C300%29&field=failure_count%28%29&field=count%28%29&field=equation%7C%28count%28%29%20%2B%20failure_count%28%29%29%20%2F%20count_if%28transaction.duration%2Cequals%2C300%29&name=Errors&project=1&query=event.type%3Aerror&statsPeriod=14d&yAxis=equation%7C%28count%28%29%20%2B%20failure_count%28%29%29%20%2F%20count_if%28transaction.duration%2Cequals%2C300%29' ); @@ -256,7 +256,7 @@ describe('Dashboards > WidgetCard', function () { ); await userEvent.click(await screen.findByLabelText('Widget actions')); - expect(screen.getByRole('link', {name: 'Open in Discover'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Open in Discover'})).toHaveAttribute( 'href', '/organizations/org-slug/discover/results/?display=top5&environment=prod&field=transaction&field=count%28%29&name=Errors&project=1&query=event.type%3Aerror&statsPeriod=14d&yAxis=count%28%29' ); @@ -292,7 +292,7 @@ describe('Dashboards > WidgetCard', function () { ); await userEvent.click(await screen.findByLabelText('Widget actions')); - expect(screen.getByRole('link', {name: 'Open in Discover'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Open in Discover'})).toHaveAttribute( 'href', '/organizations/org-slug/discover/results/?environment=prod&field=p99%28measurements.custom.measurement%29&name=Errors&project=1&query=&statsPeriod=14d&yAxis=p99%28measurements.custom.measurement%29' ); diff --git a/static/app/views/dashboards/widgetCard/issueWidgetCard.spec.tsx b/static/app/views/dashboards/widgetCard/issueWidgetCard.spec.tsx index 4bdcd12a491a2b..d476c85e3ba4bc 100644 --- a/static/app/views/dashboards/widgetCard/issueWidgetCard.spec.tsx +++ b/static/app/views/dashboards/widgetCard/issueWidgetCard.spec.tsx @@ -174,7 +174,7 @@ describe('Dashboards > IssueWidgetCard', function () { await userEvent.click(await screen.findByLabelText('Widget actions')); expect(screen.getByText('Duplicate Widget')).toBeInTheDocument(); - expect(screen.getByRole('link', {name: 'Open in Issues'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Open in Issues'})).toHaveAttribute( 'href', '/organizations/org-slug/issues/?environment=prod&project=1&query=event.type%3Adefault&sort=freq&statsPeriod=14d' ); diff --git a/static/app/views/discover/queryList.spec.tsx b/static/app/views/discover/queryList.spec.tsx index 03bd268f592e42..bb25137080142b 100644 --- a/static/app/views/discover/queryList.spec.tsx +++ b/static/app/views/discover/queryList.spec.tsx @@ -526,11 +526,15 @@ describe('Discover > QueryList', function () { const contextMenu = await screen.findByTestId('menu-trigger'); expect(contextMenu).toBeInTheDocument(); - expect(screen.queryByTestId('add-to-dashboard')).not.toBeInTheDocument(); + expect( + screen.queryByRole('menuitemradio', {name: 'Add to Dashboard'}) + ).not.toBeInTheDocument(); await userEvent.click(contextMenu); - const addToDashboardMenuItem = await screen.findByTestId('add-to-dashboard'); + const addToDashboardMenuItem = await screen.findByRole('menuitemradio', { + name: 'Add to Dashboard', + }); await userEvent.click(addToDashboardMenuItem); @@ -595,11 +599,15 @@ describe('Discover > QueryList', function () { const contextMenu = await screen.findByTestId('menu-trigger'); expect(contextMenu).toBeInTheDocument(); - expect(screen.queryByTestId('add-to-dashboard')).not.toBeInTheDocument(); + expect( + screen.queryByRole('menuitemradio', {name: 'Add to Dashboard'}) + ).not.toBeInTheDocument(); await userEvent.click(contextMenu); - const addToDashboardMenuItem = await screen.findByTestId('add-to-dashboard'); + const addToDashboardMenuItem = await screen.findByRole('menuitemradio', { + name: 'Add to Dashboard', + }); await userEvent.click(addToDashboardMenuItem); @@ -666,11 +674,15 @@ describe('Discover > QueryList', function () { const contextMenu = await screen.findByTestId('menu-trigger'); expect(contextMenu).toBeInTheDocument(); - expect(screen.queryByTestId('add-to-dashboard')).not.toBeInTheDocument(); + expect( + screen.queryByRole('menuitemradio', {name: 'Add to Dashboard'}) + ).not.toBeInTheDocument(); await userEvent.click(contextMenu); - const addToDashboardMenuItem = await screen.findByTestId('add-to-dashboard'); + const addToDashboardMenuItem = await screen.findByRole('menuitemradio', { + name: 'Add to Dashboard', + }); await userEvent.click(addToDashboardMenuItem); diff --git a/static/app/views/explore/toolbar/index.spec.tsx b/static/app/views/explore/toolbar/index.spec.tsx index 8d1a1e0478e83a..39ee2c42e28fb0 100644 --- a/static/app/views/explore/toolbar/index.spec.tsx +++ b/static/app/views/explore/toolbar/index.spec.tsx @@ -605,14 +605,15 @@ describe('ExploreToolbar', function () { const section = screen.getByTestId('section-save-as'); await userEvent.click(within(section).getByText(/Save as/)); - await userEvent.hover(within(section).getByText('An Alert for')); - await userEvent.click(screen.getByText('count(spans)')); + await userEvent.hover( + within(section).getByRole('menuitemradio', {name: 'An Alert for'}) + ); + await userEvent.click( + await within(section).findByRole('menuitemradio', {name: 'count(spans)'}) + ); expect(router.push).toHaveBeenCalledWith({ - pathname: '/organizations/org-slug/alerts/new/metric/', - query: expect.objectContaining({ - aggregate: 'count(span.duration)', - dataset: 'events_analytics_platform', - }), + pathname: + '/organizations/org-slug/alerts/new/metric/?aggregate=count%28span.duration%29&dataset=events_analytics_platform&eventTypes=transaction&interval=1h&project=proj-slug&query=&statsPeriod=7d', }); }); diff --git a/static/app/views/issueDetails/groupEventCarousel.spec.tsx b/static/app/views/issueDetails/groupEventCarousel.spec.tsx index 66387b7e0e42f9..732802f29b1e75 100644 --- a/static/app/views/issueDetails/groupEventCarousel.spec.tsx +++ b/static/app/views/issueDetails/groupEventCarousel.spec.tsx @@ -5,7 +5,7 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {RouterFixture} from 'sentry-fixture/routerFixture'; import {UserFixture} from 'sentry-fixture/user'; -import {render, screen, userEvent, within} from 'sentry-test/reactTestingLibrary'; +import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; import ConfigStore from 'sentry/stores/configStore'; import * as useMedia from 'sentry/utils/useMedia'; @@ -210,9 +210,7 @@ describe('GroupEventCarousel', () => { await userEvent.click(screen.getByRole('button', {name: /event actions/i})); expect( - within(screen.getByRole('menuitemradio', {name: /full event details/i})).getByRole( - 'link' - ) + screen.getByRole('menuitemradio', {name: /full event details/i}) ).toHaveAttribute('href', `/organizations/org-slug/discover/project-slug:event-id/`); }); diff --git a/static/app/views/issueDetails/groupTags/groupTagValues.spec.tsx b/static/app/views/issueDetails/groupTags/groupTagValues.spec.tsx index e5016ad825bb98..0b404ad4c2c7b1 100644 --- a/static/app/views/issueDetails/groupTags/groupTagValues.spec.tsx +++ b/static/app/views/issueDetails/groupTags/groupTagValues.spec.tsx @@ -4,13 +4,7 @@ import {TagsFixture} from 'sentry-fixture/tags'; import {TagValuesFixture} from 'sentry-fixture/tagvalues'; import {initializeOrg} from 'sentry-test/initializeOrg'; -import { - render, - screen, - userEvent, - waitFor, - within, -} from 'sentry-test/reactTestingLibrary'; +import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; import ProjectsStore from 'sentry/stores/projectsStore'; import {GroupTagValues} from 'sentry/views/issueDetails/groupTags/groupTagValues'; @@ -117,9 +111,7 @@ describe('GroupTagValues', () => { await userEvent.click(await screen.findByRole('button', {name: 'More'})); await userEvent.click( - within( - screen.getByRole('menuitemradio', {name: 'Search All Issues with Tag Value'}) - ).getByRole('link') + screen.getByRole('menuitemradio', {name: 'Search All Issues with Tag Value'}) ); expect(router.push).toHaveBeenCalledWith({ diff --git a/static/app/views/issueDetails/groupTags/tagDetailsDrawerContent.spec.tsx b/static/app/views/issueDetails/groupTags/tagDetailsDrawerContent.spec.tsx index 94473444bd488e..ec3a98dc23bd24 100644 --- a/static/app/views/issueDetails/groupTags/tagDetailsDrawerContent.spec.tsx +++ b/static/app/views/issueDetails/groupTags/tagDetailsDrawerContent.spec.tsx @@ -1,3 +1,4 @@ +import * as qs from 'query-string'; import {GroupFixture} from 'sentry-fixture/group'; import {OrganizationFixture} from 'sentry-fixture/organization'; import {TagsFixture} from 'sentry-fixture/tags'; @@ -24,6 +25,14 @@ jest.mock('sentry/utils/useNavigate', () => ({ const group = GroupFixture(); const tags = TagsFixture(); +const makeInitialRouterConfig = (tagKey: string) => ({ + location: { + pathname: `/organizations/org-slug/issues/1/tags/${tagKey}/`, + query: {}, + }, + route: '/organizations/:orgId/issues/:groupId/tags/:tagKey/', +}); + function init(tagKey: string) { return initializeOrg({ router: { @@ -125,28 +134,25 @@ describe('TagDetailsDrawerContent', () => { }); it('navigates to issue details events tab with correct query params', async () => { - const {router} = init('user'); - MockApiClient.addMockResponse({ url: '/organizations/org-slug/issues/1/tags/user/values/', body: TagValuesFixture(), }); render(, { - router, - deprecatedRouterMocks: true, + initialRouterConfig: makeInitialRouterConfig('user'), }); await userEvent.click( await screen.findByRole('button', {name: 'Tag Value Actions Menu'}) ); - await userEvent.click( - await screen.findByRole('link', {name: 'View other events with this tag value'}) + expect( + screen.getByRole('menuitemradio', { + name: 'View other events with this tag value', + }) + ).toHaveAttribute( + 'href', + '/organizations/org-slug/issues/1/events/?query=user.username%3Adavid' ); - - expect(router.push).toHaveBeenCalledWith({ - pathname: '/organizations/org-slug/issues/1/events/', - query: {query: 'user.username:david'}, - }); }); it('navigates to discover with issue + tag query', async () => { @@ -168,20 +174,25 @@ describe('TagDetailsDrawerContent', () => { await userEvent.click( await screen.findByRole('button', {name: 'Tag Value Actions Menu'}) ); - await userEvent.click(await screen.findByRole('link', {name: 'Open in Discover'})); - - expect(router.push).toHaveBeenCalledWith({ - pathname: '/organizations/org-slug/discover/results/', - query: { - dataset: 'errors', - field: ['title', 'release', 'environment', 'user.display', 'timestamp'], - interval: '1m', - name: 'RequestError: GET /issues/ 404', - project: '2', - query: 'issue:JAVASCRIPT-6QS user.username:david', - statsPeriod: '14d', - yAxis: ['count()', 'count_unique(user)'], - }, + + const discoverMenuItem = screen.getByRole('menuitemradio', { + name: 'Open in Discover', + }); + expect(discoverMenuItem).toBeInTheDocument(); + + const link = new URL(discoverMenuItem.getAttribute('href') ?? '', 'http://localhost'); + expect(link.pathname).toBe('/organizations/org-slug/discover/results/'); + const discoverQueryParams = qs.parse(link.search); + + expect(discoverQueryParams).toEqual({ + dataset: 'errors', + field: ['title', 'release', 'environment', 'user.display', 'timestamp'], + interval: '1m', + name: 'RequestError: GET /issues/ 404', + project: '2', + query: 'issue:JAVASCRIPT-6QS user.username:david', + statsPeriod: '14d', + yAxis: ['count()', 'count_unique(user)'], }); }); diff --git a/static/app/views/nav/orgDropdown.spec.tsx b/static/app/views/nav/orgDropdown.spec.tsx index 3b203fb6464fdc..0cdbd73ae552db 100644 --- a/static/app/views/nav/orgDropdown.spec.tsx +++ b/static/app/views/nav/orgDropdown.spec.tsx @@ -24,15 +24,14 @@ describe('OrgDropdown', function () { expect(screen.getByText('org-slug')).toBeInTheDocument(); expect(screen.getByText('0 Projects')).toBeInTheDocument(); - expect(screen.getByRole('link', {name: 'Organization Settings'})).toHaveAttribute( - 'href', - `/organizations/${organization.slug}/settings/` - ); - expect(screen.getByRole('link', {name: 'Members'})).toHaveAttribute( + expect( + screen.getByRole('menuitemradio', {name: 'Organization Settings'}) + ).toHaveAttribute('href', `/organizations/${organization.slug}/settings/`); + expect(screen.getByRole('menuitemradio', {name: 'Members'})).toHaveAttribute( 'href', `/organizations/${organization.slug}/settings/members/` ); - expect(screen.getByRole('link', {name: 'Teams'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'Teams'})).toHaveAttribute( 'href', `/organizations/${organization.slug}/settings/teams/` ); @@ -52,11 +51,11 @@ describe('OrgDropdown', function () { await userEvent.hover(screen.getByText('Switch Organization')); - expect(await screen.findByRole('link', {name: /org-1/})).toHaveAttribute( + expect(await screen.findByRole('menuitemradio', {name: /org-1/})).toHaveAttribute( 'href', `/organizations/org-1/issues/` ); - expect(await screen.findByRole('link', {name: /org-2/})).toHaveAttribute( + expect(await screen.findByRole('menuitemradio', {name: /org-2/})).toHaveAttribute( 'href', `/organizations/org-2/issues/` ); diff --git a/static/app/views/nav/userDropdown.spec.tsx b/static/app/views/nav/userDropdown.spec.tsx index 544d63e458023d..cde7077317a1bd 100644 --- a/static/app/views/nav/userDropdown.spec.tsx +++ b/static/app/views/nav/userDropdown.spec.tsx @@ -18,7 +18,7 @@ describe('UserDropdown', function () { expect(screen.getByText('Foo Bar')).toBeInTheDocument(); expect(screen.getByText('foo@example.com')).toBeInTheDocument(); - expect(screen.getByRole('link', {name: 'User Settings'})).toHaveAttribute( + expect(screen.getByRole('menuitemradio', {name: 'User Settings'})).toHaveAttribute( 'href', '/settings/account/' ); @@ -46,7 +46,7 @@ describe('UserDropdown', function () { await userEvent.click(screen.getByRole('button', {name: 'foo@example.com'})); - expect(screen.queryByRole('link', {name: 'Admin'})).not.toBeInTheDocument(); + expect(screen.queryByRole('menuitemradio', {name: 'Admin'})).not.toBeInTheDocument(); }); it('shows admin link if user is admin', async function () { @@ -56,6 +56,9 @@ describe('UserDropdown', function () { await userEvent.click(screen.getByRole('button', {name: 'foo@example.com'})); - expect(screen.getByRole('link', {name: 'Admin'})).toHaveAttribute('href', `/manage/`); + expect(screen.getByRole('menuitemradio', {name: 'Admin'})).toHaveAttribute( + 'href', + `/manage/` + ); }); }); From 161e95adab8be4bd41c0dee95f2fb5ac8a89a0bb Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Thu, 1 May 2025 13:32:05 -0700 Subject: [PATCH 3/3] Update chonk styles --- static/app/components/core/menuListItem/index.chonk.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/static/app/components/core/menuListItem/index.chonk.tsx b/static/app/components/core/menuListItem/index.chonk.tsx index 9c91e99ede9a05..4b237c30693b3f 100644 --- a/static/app/components/core/menuListItem/index.chonk.tsx +++ b/static/app/components/core/menuListItem/index.chonk.tsx @@ -71,8 +71,12 @@ export const ChonkInnerWrap = chonkStyled('div', { font-size: ${p => p.theme.form[p.size ?? 'md'].fontSize}; &, - &:hover { + &:hover, + &:focus, + &:focus-visible { color: ${getTextColor}; + box-shadow: none; + outline: none; } ${p => p.disabled && `cursor: default;`}