From 5604fa6cb16e50b330930b98de0e0a5cdce0a2e4 Mon Sep 17 00:00:00 2001 From: Marek Mihok Date: Tue, 14 Mar 2023 19:54:20 +0100 Subject: [PATCH 1/5] fix: do not open menu on esc key #1866 --- ui/src/menu.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ui/src/menu.tsx b/ui/src/menu.tsx index abb99ca982..63948adbd5 100644 --- a/ui/src/menu.tsx +++ b/ui/src/menu.tsx @@ -46,7 +46,8 @@ export const XMenu = ({ model }: { model: Menu }) => { { name, items, icon, image, label } = model, ref = React.useRef(null), [isMenuHidden, setIsMenuHidden] = React.useState(true), - toggleMenu = () => setIsMenuHidden(isHidden => !isHidden) + toggleMenu = () => setIsMenuHidden(isHidden => !isHidden), + dismissMenu = () => setIsMenuHidden(true) return ( // HACK: Marker css class. @@ -58,7 +59,7 @@ export const XMenu = ({ model }: { model: Menu }) => { items={toCommands(items)} target={ref} hidden={isMenuHidden} - onDismiss={toggleMenu} + onDismiss={dismissMenu} isBeakVisible directionalHint={Fluent.DirectionalHint.bottomRightEdge} calloutProps={{ styles: { beak: { border: border(1, cssVar('$neutralQuaternaryAlt')) } } }} From 3af1c69d169a05a9294313b0ebc52a56bcc98d6c Mon Sep 17 00:00:00 2001 From: Marek Mihok Date: Wed, 15 Mar 2023 14:21:20 +0100 Subject: [PATCH 2/5] chore: add basic tests #1866 --- ui/src/menu.test.tsx | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 ui/src/menu.test.tsx diff --git a/ui/src/menu.test.tsx b/ui/src/menu.test.tsx new file mode 100644 index 0000000000..8a061c1b33 --- /dev/null +++ b/ui/src/menu.test.tsx @@ -0,0 +1,50 @@ +// Copyright 2020 H2O.ai, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { fireEvent, render, waitFor } from '@testing-library/react' +import React from 'react' +import { Menu, XMenu } from './menu' + +const name = 'menu' + +describe('Menu.tsx', () => { + + describe('Menu', () => { + const menuProps: Menu = { + name, + label: 'Menu', + items: [ + { name: 'item1', label: 'Item 1' }, + { name: 'item2', label: 'Item 2' } + ] + } + + it('Renders data-test attr', () => { + const { queryByTestId } = render() + expect(queryByTestId(name)).toBeInTheDocument() + }) + + it('Does not render data-test attr - name not specified', () => { + const { queryByTestId } = render() + expect(queryByTestId(name)).not.toBeInTheDocument() + }) + + // it('Opens menu when clicked', () => { + // const { container, getByRole } = render() + // expect(getByRole('menu')).not.toBeVisible() + // fireEvent.click(container.querySelector('button')!) + // expect(getByRole('menu')).toBeVisible() + // }) + }) +}) \ No newline at end of file From f4eba8135745b802c3684f85206250fccf9d9947 Mon Sep 17 00:00:00 2001 From: Marek Mihok Date: Wed, 15 Mar 2023 14:37:41 +0100 Subject: [PATCH 3/5] chore: add menu open/close tests #1866 --- ui/src/menu.test.tsx | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/ui/src/menu.test.tsx b/ui/src/menu.test.tsx index 8a061c1b33..a0a202b570 100644 --- a/ui/src/menu.test.tsx +++ b/ui/src/menu.test.tsx @@ -40,11 +40,28 @@ describe('Menu.tsx', () => { expect(queryByTestId(name)).not.toBeInTheDocument() }) - // it('Opens menu when clicked', () => { - // const { container, getByRole } = render() - // expect(getByRole('menu')).not.toBeVisible() - // fireEvent.click(container.querySelector('button')!) - // expect(getByRole('menu')).toBeVisible() - // }) + it('Opens menu when clicked', () => { + const { container, queryByRole } = render() + expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(container.querySelector('button')!) + expect(queryByRole('menu')).toBeInTheDocument() + }) + + it('Closes menu on Esc key', () => { + const { container, queryByRole } = render() + expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(container.querySelector('button')!) + expect(queryByRole('menu')).toBeInTheDocument() + fireEvent.keyDown(document, { key: 'Escape' }) + // TODO: Figure out why this doesn't work + expect(queryByRole('menu')).not.toBeInTheDocument() + }) + + it('Does not open menu on Esc key', () => { + const { queryByRole } = render() + expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.keyDown(document, { key: 'Escape' }) + expect(queryByRole('menu')).not.toBeInTheDocument() + }) }) }) \ No newline at end of file From 265f24e25e4ed2ba2084fc0a8ce7ab0089ec51bf Mon Sep 17 00:00:00 2001 From: Marek Mihok Date: Thu, 16 Mar 2023 18:00:00 +0100 Subject: [PATCH 4/5] chore: add test cases to make sure Esc key only closes the menu #1866 --- ui/src/menu.test.tsx | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/ui/src/menu.test.tsx b/ui/src/menu.test.tsx index a0a202b570..24944aa281 100644 --- a/ui/src/menu.test.tsx +++ b/ui/src/menu.test.tsx @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { fireEvent, render, waitFor } from '@testing-library/react' import React from 'react' +import { fireEvent, render } from '@testing-library/react' import { Menu, XMenu } from './menu' +import { KeyCodes } from '@fluentui/react' const name = 'menu' @@ -40,27 +41,44 @@ describe('Menu.tsx', () => { expect(queryByTestId(name)).not.toBeInTheDocument() }) - it('Opens menu when clicked', () => { - const { container, queryByRole } = render() + it('Opens menu when clicked on the button', () => { + const { queryByRole } = render() + expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') + expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) + expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') + expect(queryByRole('menu')).toBeInTheDocument() + }) + + it('Closes open menu when clicked on the button', () => { + const { queryByRole } = render() + expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() - fireEvent.click(container.querySelector('button')!) + fireEvent.click(document.querySelector('button')!) + expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') expect(queryByRole('menu')).toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) + expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') + expect(queryByRole('menu')).not.toBeInTheDocument() }) it('Closes menu on Esc key', () => { - const { container, queryByRole } = render() + const { queryByRole } = render() + expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() - fireEvent.click(container.querySelector('button')!) + fireEvent.click(document.querySelector('button')!) + expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') expect(queryByRole('menu')).toBeInTheDocument() - fireEvent.keyDown(document, { key: 'Escape' }) - // TODO: Figure out why this doesn't work + // FluentUI uses the deprecated 'which' property instead of the 'key' prop. + fireEvent.keyDown(window, { which: KeyCodes.escape }) + expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() }) it('Does not open menu on Esc key', () => { const { queryByRole } = render() expect(queryByRole('menu')).not.toBeInTheDocument() - fireEvent.keyDown(document, { key: 'Escape' }) + fireEvent.keyDown(window, { which: KeyCodes.escape }) expect(queryByRole('menu')).not.toBeInTheDocument() }) }) From e23a9361e0c6ccfc6db95f59a87987a931baed30 Mon Sep 17 00:00:00 2001 From: Marek Mihok Date: Thu, 16 Mar 2023 18:05:50 +0100 Subject: [PATCH 5/5] chore: refactor test cases #1866 --- ui/src/menu.test.tsx | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/ui/src/menu.test.tsx b/ui/src/menu.test.tsx index 24944aa281..3e2c4077ba 100644 --- a/ui/src/menu.test.tsx +++ b/ui/src/menu.test.tsx @@ -43,41 +43,40 @@ describe('Menu.tsx', () => { it('Opens menu when clicked on the button', () => { const { queryByRole } = render() - expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) - expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') expect(queryByRole('menu')).toBeInTheDocument() }) it('Closes open menu when clicked on the button', () => { const { queryByRole } = render() - expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) - expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') expect(queryByRole('menu')).toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) - expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() }) it('Closes menu on Esc key', () => { const { queryByRole } = render() - expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() + fireEvent.click(document.querySelector('button')!) - expect(document.querySelector('.ms-Callout')).not.toHaveAttribute('hidden') expect(queryByRole('menu')).toBeInTheDocument() - // FluentUI uses the deprecated 'which' property instead of the 'key' prop. + + // FluentUI uses a deprecated 'which' property instead of the 'key' prop. fireEvent.keyDown(window, { which: KeyCodes.escape }) - expect(document.querySelector('.ms-Callout')).toHaveAttribute('hidden') expect(queryByRole('menu')).not.toBeInTheDocument() }) it('Does not open menu on Esc key', () => { const { queryByRole } = render() expect(queryByRole('menu')).not.toBeInTheDocument() + + // FluentUI uses a deprecated 'which' property instead of the 'key' prop. fireEvent.keyDown(window, { which: KeyCodes.escape }) expect(queryByRole('menu')).not.toBeInTheDocument() })