From 6528bcb368b32328e3bfed156f9f928fd6c46016 Mon Sep 17 00:00:00 2001 From: nvms Date: Wed, 5 Feb 2025 14:43:03 -0500 Subject: [PATCH 01/10] use popover api --- frontend/src/components/Menu.js | 85 ++++++++++++------- frontend/src/components/Menu.scss | 12 ++- .../components/WidgetContainerTitleGroup.js | 2 + frontend/src/widgets/HorizontalTableWidget.js | 2 +- .../src/widgets/HorizontalTableWidget.scss | 10 +-- 5 files changed, 67 insertions(+), 44 deletions(-) diff --git a/frontend/src/components/Menu.js b/frontend/src/components/Menu.js index 4696a064de..523bc15747 100644 --- a/frontend/src/components/Menu.js +++ b/frontend/src/components/Menu.js @@ -1,3 +1,4 @@ +/* eslint-disable react/no-unknown-property */ /* Context menu shows a list of actions the user can select to perform actions. The actions are hidden until the user opens the menu (the three dot icon). The menu is automatically @@ -28,9 +29,10 @@ function Menu({ fixed, }) { const [shown, updateShown] = useState(false); - const [menuPosition, updateMenuPosition] = useState({}); + const [, updateMenuPosition] = useState({}); const defaultClass = 'smart-hub--menu'; const buttonRef = useRef(null); + const popoverRef = useRef(null); const onEscape = useCallback((event) => { if (event.keyCode === ESCAPE_KEY_CODE) { @@ -48,35 +50,28 @@ function Menu({ const recordButtonPositionAndUpdateMenu = useCallback(() => { // set initial postition - if (fixed && buttonRef.current && buttonRef.current.getBoundingClientRect) { - // get the button's position - const { - top, - height, - left: l, - width, - } = buttonRef.current.getBoundingClientRect(); - - // we could be progratically calculating the height and width offset numbers - // but a little manual work up front will save on performance in the browser + if (!(fixed && buttonRef.current && buttonRef.current.getBoundingClientRect)) { + return; + } - let leftPos = l + width; + // get the button's position + const { + x, + y, + height, + width, + } = buttonRef.current.getBoundingClientRect(); - // left = the menu opens to the left of the button instead of the right - if (left) { - leftPos = l + width - menuWidthOffset; - } + // we could be progratically calculating the height and width offset numbers + // but a little manual work up front will save on performance in the browser - // top = the menu opens above the button instead of below - let topPos = top + height; + const leftPos = left ? x + width - menuWidthOffset : x + width; - if (up) { - topPos = top - height - menuHeightOffset; - } + // top = the menu opens above the button instead of below + const topPos = up ? y - height - menuHeightOffset : y + height; - // update the CSS - updateMenuPosition({ top: `${topPos}px`, left: `${leftPos}px` }); - } + // update the CSS + updateMenuPosition({ top: `${topPos}px`, left: `${leftPos}px`, margin: 0 }); }, [fixed, left, menuHeightOffset, menuWidthOffset, up]); // watch for window scroll @@ -124,13 +119,34 @@ function Menu({ const menuClass = `${defaultClass} shadow-1 z-top ${positionClass} ${placementClass}`; const onClick = () => { - // we don't need to check anything before calling this - if fixed is false, it won't do anything - recordButtonPositionAndUpdateMenu(); - - // toggle the menu visibility + // toggle the menu visibility using the Popover API + if (popoverRef.current) { + popoverRef.current.togglePopover(); + } updateShown((previous) => !previous); }; + const getPopoverPosition = () => { + if (buttonRef.current) { + const { + x, + y, + height, + width, + } = buttonRef.current.getBoundingClientRect(); + + // we could be progratically calculating the height and width offset numbers + // but a little manual work up front will save on performance in the browser + + const leftPos = left ? x + width - menuWidthOffset : x + width; + + // top = the menu opens above the button instead of below + const topPos = up ? y - height - menuHeightOffset : y + height; + return { top: `${topPos}px`, left: `${leftPos}px`, margin: 0 }; + } + return {}; + }; + return (
{buttonText} {shown && ( -
+ diff --git a/frontend/src/widgets/HorizontalTableWidget.js b/frontend/src/widgets/HorizontalTableWidget.js index 0cdd50e931..7326d503c4 100644 --- a/frontend/src/widgets/HorizontalTableWidget.js +++ b/frontend/src/widgets/HorizontalTableWidget.js @@ -269,7 +269,7 @@ export default function HorizontalTableWidget( fixed label="Actions for Communication Log" menuItems={r.actions} - menuWidthOffset={110} + menuWidthOffset={100} /> ) : null} diff --git a/frontend/src/widgets/HorizontalTableWidget.scss b/frontend/src/widgets/HorizontalTableWidget.scss index 999eb6cfe8..fd32a18c10 100644 --- a/frontend/src/widgets/HorizontalTableWidget.scss +++ b/frontend/src/widgets/HorizontalTableWidget.scss @@ -27,15 +27,7 @@ min-width: 90px; } - // Because putting the border on the sticky element doesn't work, and box-shadow - // isn't ideal because of z-indexing weirdness that happens when a ContextMenu - // is above the border. - .smarthub-horizontal-table-widget .smarthub-horizontal-table-last-column::before { - content: ''; - position: absolute; - top: 0; - left: 0; - height: 100%; + .smarthub-horizontal-table-widget .usa-table .smarthub-horizontal-table-last-column { border-left: 1px solid black; } From 435aea409e36c62876729f99d04b5b5fb4699e1b Mon Sep 17 00:00:00 2001 From: nvms Date: Wed, 5 Feb 2025 15:15:22 -0500 Subject: [PATCH 02/10] revert css change --- frontend/src/widgets/HorizontalTableWidget.scss | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/frontend/src/widgets/HorizontalTableWidget.scss b/frontend/src/widgets/HorizontalTableWidget.scss index fd32a18c10..999eb6cfe8 100644 --- a/frontend/src/widgets/HorizontalTableWidget.scss +++ b/frontend/src/widgets/HorizontalTableWidget.scss @@ -27,7 +27,15 @@ min-width: 90px; } - .smarthub-horizontal-table-widget .usa-table .smarthub-horizontal-table-last-column { + // Because putting the border on the sticky element doesn't work, and box-shadow + // isn't ideal because of z-indexing weirdness that happens when a ContextMenu + // is above the border. + .smarthub-horizontal-table-widget .smarthub-horizontal-table-last-column::before { + content: ''; + position: absolute; + top: 0; + left: 0; + height: 100%; border-left: 1px solid black; } From e3501d482b4dacd77caba9481a9471342d41896f Mon Sep 17 00:00:00 2001 From: nvms Date: Wed, 5 Feb 2025 15:15:33 -0500 Subject: [PATCH 03/10] refactor for simplification --- frontend/src/components/Menu.js | 40 ++++++--------------------------- 1 file changed, 7 insertions(+), 33 deletions(-) diff --git a/frontend/src/components/Menu.js b/frontend/src/components/Menu.js index 523bc15747..d6667773c2 100644 --- a/frontend/src/components/Menu.js +++ b/frontend/src/components/Menu.js @@ -8,6 +8,7 @@ */ import React, { useState, useEffect, useCallback, useRef, + useLayoutEffect, } from 'react'; import PropTypes from 'prop-types'; import { Button } from '@trussworks/react-uswds'; @@ -29,7 +30,7 @@ function Menu({ fixed, }) { const [shown, updateShown] = useState(false); - const [, updateMenuPosition] = useState({}); + const [menuPosition, updateMenuPosition] = useState({}); const defaultClass = 'smart-hub--menu'; const buttonRef = useRef(null); const popoverRef = useRef(null); @@ -49,12 +50,10 @@ function Menu({ }, [onEscape]); const recordButtonPositionAndUpdateMenu = useCallback(() => { - // set initial postition if (!(fixed && buttonRef.current && buttonRef.current.getBoundingClientRect)) { return; } - // get the button's position const { x, y, @@ -62,24 +61,16 @@ function Menu({ width, } = buttonRef.current.getBoundingClientRect(); - // we could be progratically calculating the height and width offset numbers - // but a little manual work up front will save on performance in the browser - const leftPos = left ? x + width - menuWidthOffset : x + width; // top = the menu opens above the button instead of below const topPos = up ? y - height - menuHeightOffset : y + height; - // update the CSS updateMenuPosition({ top: `${topPos}px`, left: `${leftPos}px`, margin: 0 }); }, [fixed, left, menuHeightOffset, menuWidthOffset, up]); // watch for window scroll useEffect(() => { - // the menu position is based on the button position, but because it is encased in a - // no-overflow div, we position it using "fixed" - // this means that it wouldn't scroll with the page, so we need to update the position - // when the user scrolls if (fixed) { window.addEventListener('scroll', recordButtonPositionAndUpdateMenu); } @@ -89,6 +80,10 @@ function Menu({ }; }, [fixed, recordButtonPositionAndUpdateMenu]); + useLayoutEffect(() => { + recordButtonPositionAndUpdateMenu(); + }, [recordButtonPositionAndUpdateMenu]); + const onBlur = (e) => { const { currentTarget } = e; @@ -126,27 +121,6 @@ function Menu({ updateShown((previous) => !previous); }; - const getPopoverPosition = () => { - if (buttonRef.current) { - const { - x, - y, - height, - width, - } = buttonRef.current.getBoundingClientRect(); - - // we could be progratically calculating the height and width offset numbers - // but a little manual work up front will save on performance in the browser - - const leftPos = left ? x + width - menuWidthOffset : x + width; - - // top = the menu opens above the button instead of below - const topPos = up ? y - height - menuHeightOffset : y + height; - return { top: `${topPos}px`, left: `${leftPos}px`, margin: 0 }; - } - return {}; - }; - return (
    {menuItems.map((item) => ( From 7f7bfd402b3cf727d44eaad0a9df5355980f8d60 Mon Sep 17 00:00:00 2001 From: nvms Date: Wed, 5 Feb 2025 16:19:21 -0500 Subject: [PATCH 04/10] dont call togglePopover --- frontend/src/components/Menu.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frontend/src/components/Menu.js b/frontend/src/components/Menu.js index d6667773c2..8c7210a505 100644 --- a/frontend/src/components/Menu.js +++ b/frontend/src/components/Menu.js @@ -114,10 +114,6 @@ function Menu({ const menuClass = `${defaultClass} shadow-1 z-top ${positionClass} ${placementClass}`; const onClick = () => { - // toggle the menu visibility using the Popover API - if (popoverRef.current) { - popoverRef.current.togglePopover(); - } updateShown((previous) => !previous); }; From 59c8e95c44182da925945df7d1ad80e93df84a7e Mon Sep 17 00:00:00 2001 From: nvms Date: Thu, 6 Feb 2025 12:57:19 -0500 Subject: [PATCH 05/10] try this selector --- tests/e2e/activity-report.spec.ts | 44 +++++++++++++++---------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/e2e/activity-report.spec.ts b/tests/e2e/activity-report.spec.ts index 3c83e2ba99..b6ffe905b2 100644 --- a/tests/e2e/activity-report.spec.ts +++ b/tests/e2e/activity-report.spec.ts @@ -110,7 +110,7 @@ async function activitySummary( if (recipients) { // select recipients for (let i = 0; i < recipients; i++) { - await page.keyboard.press('Enter'); + await page.keyboard.press('Enter'); } } else { await page.keyboard.press('Enter'); @@ -250,7 +250,7 @@ test.describe('Activity Report', () => { await page.keyboard.press('ArrowDown'); await page.keyboard.press('Enter'); await blur(page); - await page. locator('[id="goalForEditing\.objectives\[0\]\.title"]').fill('g2o1'); + await page. locator('[id="goalForEditing\.objectives\[0\]\.title"]').fill('g2o1'); await page.getByText('Topics *').click() await page.keyboard.press('ArrowDown'); await page.keyboard.press('Enter'); @@ -273,7 +273,7 @@ test.describe('Activity Report', () => { // assert the goals and objectives section is complete let sideNavTextContent = await page.locator('#activityReportSideNav-goals-and-objectives .page-state').textContent(); - + await page.waitForTimeout(10000); expect(sideNavTextContent?.match(/Complete/i)).toBeTruthy(); @@ -297,7 +297,7 @@ test.describe('Activity Report', () => { sideNavTextContent = await page.locator('#activityReportSideNav-goals-and-objectives .page-state').textContent(); expect(sideNavTextContent?.match(/in progress/i)).toBeTruthy(); - + // save the first goal await page.getByRole('button', { name: 'Save goal' }).click(); @@ -464,7 +464,7 @@ test.describe('Activity Report', () => { .getByRole('button', { name: 'Actions for goal' }) .click(); // click on the 'Edit' button for 'g1' and verify the correct data is displayed - await page.getByText('g1', { exact: true }).locator('..').locator('..').locator('..') + await page.getByText('g1', { exact: true }).locator('..').locator('..') .getByRole('button', { name: 'Edit' }) .click(); @@ -482,7 +482,7 @@ test.describe('Activity Report', () => { .getByRole('button', { name: 'Actions for goal' }) .click(); // click on the 'Edit' button for 'g1' and verify the correct data is displayed - await page.getByText('g2', { exact: true }).locator('..').locator('..').locator('..') + await page.getByText('g2', { exact: true }).locator('..').locator('..') .getByRole('button', { name: 'Edit' }) .click(); @@ -513,7 +513,7 @@ test.describe('Activity Report', () => { // enter goal name await page.getByLabel('Recipient\'s goal *').fill('This is a goal for multiple grants'); await page.getByRole('button', { name: 'Save and continue' }).click(); - + await page.waitForTimeout(5000); // select recipients @@ -576,9 +576,9 @@ test.describe('Activity Report', () => { await page.getByText('Topics *').click() await page.keyboard.press('ArrowDown'); - await page.keyboard.press('Enter'); + await page.keyboard.press('Enter'); await blur(page); - + const supportType = page.getByRole('combobox', { name: /Support type/i }); await supportType.selectOption('Implementing'); @@ -650,12 +650,12 @@ test.describe('Activity Report', () => { // create a new report await page.getByRole('link', { name: 'Activity Reports' }).click(); await page.getByRole('button', { name: '+ New Activity Report' }).click(); - + const heading = page.getByRole('heading', { name: /activity report for region \d/i }); - const regionNumber = await heading.textContent().then((text) => text!.match(/\d/)![0]); + const regionNumber = await heading.textContent().then((text) => text!.match(/\d/)![0]); await activitySummary(page, { recipients: 2, ttaType: 'Training' }); - + // select two recipients await page.locator('label').filter({ hasText: 'Other entity' }).click(); await page.locator('#activityRecipients div').filter({ hasText: '- Select -' }).nth(1).click(); @@ -666,7 +666,7 @@ test.describe('Activity Report', () => { await page.getByRole('button', { name: 'Goals and objectives Not started' }).click(); await page.getByRole('button', { name: 'Supporting attachments Not started' }).click(); await page.getByRole('button', { name: 'Next steps Not started' }).click(); - await page.getByRole('button', { name: 'Review and submit' }).click(); + await page.getByRole('button', { name: 'Review and submit' }).click(); await page.getByRole('button', { name: 'Activity summary Complete' }).click(); // select participants @@ -713,7 +713,7 @@ test.describe('Activity Report', () => { // skip supporting attachments await page.getByRole('button', { name: 'Save and continue' }).click(); - + // fill out next steps const isOtherEntity = true; await nextSteps(page, isOtherEntity); @@ -723,7 +723,7 @@ test.describe('Activity Report', () => { await page.getByLabel(/Approving manager/i).focus(); await page.keyboard.press('ArrowDown'); await page.keyboard.press('Enter'); - + // extract the AR number from the URL: const url = page.url(); const arNumber = url.split('/').find((part) => /^\d+$/.test(part)); @@ -755,14 +755,14 @@ test.describe('Activity Report', () => { await page.getByLabel('Topics *').press('ArrowDown'); await page.getByLabel('Topics *').press('Enter'); await page.getByLabel('Topics *').press('Tab'); - + await blur(page); await page.getByText('TTA provided *Normal').click(); await page.getByRole('textbox', { name: 'TTA provided for objective, required' }).press('Enter'); await page.getByText('Support type *').click(); await page.getByRole('combobox', { name: 'Support type' }).selectOption('Introducing'); - + await page.getByRole('button', { name: 'Save objectives' }).click(); await page.getByRole('button', { name: 'Save and continue' }).click(); await page.getByRole('button', { name: 'Goals and objectives Complete' }).click(); @@ -888,11 +888,11 @@ test.describe('Activity Report', () => { await activitySummary(page); const p = page.waitForURL('**/goals-objectives'); - + // visit the goals & objectives page await page.getByRole('button', { name: 'Goals and objectives Not Started' }).click(); - await p; + await p; // create the goal await page.waitForTimeout(5000); @@ -904,14 +904,14 @@ test.describe('Activity Report', () => { // create the objective await page.getByText('Select TTA objective *- Select -').click(); await page.keyboard.press('ArrowDown'); - await page.keyboard.press('Enter'); - + await page.keyboard.press('Enter'); + await page.locator('[id="goalForEditing\.objectives\[0\]\.title"]').fill('Test objective for preserving objectives'); await blur(page); await page.getByText('Topics *').click() await page.keyboard.press('ArrowDown'); - await page.keyboard.press('Enter'); + await page.keyboard.press('Enter'); await page.getByRole('textbox', { name: /TTA provided for objective/i }).locator('div').nth(2).click(); await page.keyboard.type('An unlikely statement'); From c04b4ec76ac7012494115687a43873195f995bfd Mon Sep 17 00:00:00 2001 From: nvms Date: Thu, 6 Feb 2025 13:03:29 -0500 Subject: [PATCH 06/10] mark all as fixed --- frontend/src/components/GoalCards/GoalCard.js | 1 + frontend/src/components/GoalForm/ReadOnlyGoal.js | 1 + .../src/components/GoalForm/ReadOnlyOtherEntityObjectives.js | 1 + frontend/src/pages/TrainingReports/components/EventCard.js | 1 + 4 files changed, 4 insertions(+) diff --git a/frontend/src/components/GoalCards/GoalCard.js b/frontend/src/components/GoalCards/GoalCard.js index 23e75e8b78..6f89918061 100644 --- a/frontend/src/components/GoalCards/GoalCard.js +++ b/frontend/src/components/GoalCards/GoalCard.js @@ -270,6 +270,7 @@ export default function GoalCard({
{ !hideGoalOptions && ( Recipient TTA goal
{menuItems.length > 0 && ( From 424bdb774f363a2da21f6b0c73fd329da78b4a13 Mon Sep 17 00:00:00 2001 From: nvms Date: Thu, 6 Feb 2025 13:17:57 -0500 Subject: [PATCH 07/10] simplify redraw stuff --- frontend/src/components/Menu.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/Menu.js b/frontend/src/components/Menu.js index 8c7210a505..167ff4b249 100644 --- a/frontend/src/components/Menu.js +++ b/frontend/src/components/Menu.js @@ -8,7 +8,6 @@ */ import React, { useState, useEffect, useCallback, useRef, - useLayoutEffect, } from 'react'; import PropTypes from 'prop-types'; import { Button } from '@trussworks/react-uswds'; @@ -80,9 +79,11 @@ function Menu({ }; }, [fixed, recordButtonPositionAndUpdateMenu]); - useLayoutEffect(() => { - recordButtonPositionAndUpdateMenu(); - }, [recordButtonPositionAndUpdateMenu]); + useEffect(() => { + if (shown) { + recordButtonPositionAndUpdateMenu(); + } + }, [recordButtonPositionAndUpdateMenu, shown]); const onBlur = (e) => { const { currentTarget } = e; From 925adf9f57b51ec136d1f60fb3a69bebd87f2da9 Mon Sep 17 00:00:00 2001 From: nvms Date: Thu, 6 Feb 2025 13:18:22 -0500 Subject: [PATCH 08/10] because these are overwritten by .usa-form styles otherwise --- frontend/src/components/Menu.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frontend/src/components/Menu.scss b/frontend/src/components/Menu.scss index a8f109e5a1..2255e184e6 100644 --- a/frontend/src/components/Menu.scss +++ b/frontend/src/components/Menu.scss @@ -1,11 +1,13 @@ @use '../colors.scss' as *; +.usa-form .usa-button.smart-hub--menu-button, .smart-hub--menu-button { display: block; text-decoration: none; width: max-content; height: 100%; color: $text-ink; + margin: 0; } .smart-hub--menu-button:hover { From a82df8d433eb40fc8d67c699d576c99c3c7c4c0f Mon Sep 17 00:00:00 2001 From: nvms Date: Thu, 6 Feb 2025 13:18:51 -0500 Subject: [PATCH 09/10] update reportrow --- frontend/src/components/ActivityReportsTable/ReportRow.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/ActivityReportsTable/ReportRow.js b/frontend/src/components/ActivityReportsTable/ReportRow.js index fc9002c26a..de50dd5dae 100644 --- a/frontend/src/components/ActivityReportsTable/ReportRow.js +++ b/frontend/src/components/ActivityReportsTable/ReportRow.js @@ -141,7 +141,13 @@ function ReportRow({ {lastSaved} {approvedAt && moment(approvedAt).format(DATE_DISPLAY_FORMAT)} - + ); From e4b6510584b052a8917d01b4e6b7c658be496ff9 Mon Sep 17 00:00:00 2001 From: nvms Date: Fri, 14 Feb 2025 12:38:31 -0500 Subject: [PATCH 10/10] restore this and see what breaks --- tests/e2e/activity-report.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/activity-report.spec.ts b/tests/e2e/activity-report.spec.ts index b6ffe905b2..e6ec409fda 100644 --- a/tests/e2e/activity-report.spec.ts +++ b/tests/e2e/activity-report.spec.ts @@ -464,7 +464,7 @@ test.describe('Activity Report', () => { .getByRole('button', { name: 'Actions for goal' }) .click(); // click on the 'Edit' button for 'g1' and verify the correct data is displayed - await page.getByText('g1', { exact: true }).locator('..').locator('..') + await page.getByText('g1', { exact: true }).locator('..').locator('..').locator('..') .getByRole('button', { name: 'Edit' }) .click(); @@ -482,7 +482,7 @@ test.describe('Activity Report', () => { .getByRole('button', { name: 'Actions for goal' }) .click(); // click on the 'Edit' button for 'g1' and verify the correct data is displayed - await page.getByText('g2', { exact: true }).locator('..').locator('..') + await page.getByText('g2', { exact: true }).locator('..').locator('..').locator('..') .getByRole('button', { name: 'Edit' }) .click();