From 708ecb74fedaae044c2a161dac00d9a25ddab122 Mon Sep 17 00:00:00 2001 From: Boris Serdiuk Date: Tue, 28 Jan 2025 11:32:04 +0100 Subject: [PATCH 1/7] fix: Incorporate split panel height when rendering stickyVerticalBottomOffset value (#3225) --- .../__integ__/app-layout-split-panel.test.ts | 27 ++++++++++++----- .../visual-refresh-toolbar/compute-layout.ts | 30 +++++++++++++++++++ .../visual-refresh-toolbar/index.tsx | 25 +++++++++++----- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/src/app-layout/__integ__/app-layout-split-panel.test.ts b/src/app-layout/__integ__/app-layout-split-panel.test.ts index de4f3fc3f3..f936d2846f 100644 --- a/src/app-layout/__integ__/app-layout-split-panel.test.ts +++ b/src/app-layout/__integ__/app-layout-split-panel.test.ts @@ -5,9 +5,12 @@ import useBrowser from '@cloudscape-design/browser-test-tools/use-browser'; import createWrapper from '../../../lib/components/test-utils/selectors'; import { scrollbarThickness } from '../../__integ__/scrollbars'; import { viewports } from './constants'; -import { AppLayoutSplitViewPage } from './utils'; +import { AppLayoutSplitViewPage, getUrlParams } from './utils'; import mobileStyles from '../../../lib/components/app-layout/mobile-toolbar/styles.selectors.js'; +import tableScrollbarStyles from '../../../lib/components/table/sticky-scrollbar/styles.selectors.js'; + +const scrollbarSelector = `.${tableScrollbarStyles['sticky-scrollbar-visible']}`; const wrapper = createWrapper().findAppLayout(); @@ -19,11 +22,7 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme => return useBrowser(async browser => { const page = new AppLayoutSplitViewPage(browser); await page.setWindowSize(viewports.desktop); - const params = new URLSearchParams({ - visualRefresh: `${theme.startsWith('refresh')}`, - appLayoutToolbar: `${theme === 'refresh-toolbar'}`, - }); - await browser.url(`${url}?${params.toString()}`); + await browser.url(`${url}?${getUrlParams(theme)}`); await page.waitForVisible(wrapper.findContentRegion().toSelector()); await testFn(page); }); @@ -274,7 +273,7 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme => }) ); - describe('interaction with table sticky header', () => { + describe('interaction with table sticky elements', () => { // bottom padding is included into the offset in VR but not in classic const splitPanelPadding = theme === 'refresh' ? 40 : 0; @@ -298,5 +297,19 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme => await expect(page.getContentOffsetBottom(theme)).resolves.toEqual(windowHeight / 2 + splitPanelPadding + 'px'); }, '#/light/app-layout/with-full-page-table-and-split-panel') ); + + test( + 'bottom split panel does not block table sticky scrollbar', + setupTest(async page => { + // open tools panel to narrow the content area and display horizontal scroll on table + await page.click(wrapper.findToolsToggle().toSelector()); + await expect(page.isClickable(scrollbarSelector)).resolves.toBe(true); + await page.click(wrapper.findSplitPanelOpenButton().toSelector()); + await expect( + page.isDisplayedInViewport(wrapper.findSplitPanel().findOpenPanelBottom().toSelector()) + ).resolves.toBe(true); + await expect(page.isClickable(scrollbarSelector)).resolves.toBe(true); + }, '#/light/app-layout/with-sticky-table-and-split-panel') + ); }); }); diff --git a/src/app-layout/visual-refresh-toolbar/compute-layout.ts b/src/app-layout/visual-refresh-toolbar/compute-layout.ts index 20bd6eb00a..084efdbbbb 100644 --- a/src/app-layout/visual-refresh-toolbar/compute-layout.ts +++ b/src/app-layout/visual-refresh-toolbar/compute-layout.ts @@ -113,6 +113,36 @@ export function computeVerticalLayout({ return { toolbar, notifications, header, drawers }; } +interface SplitPanelOffsetInput { + hasSplitPanel: boolean; + placement: AppLayoutPropsWithDefaults['placement']; + splitPanelPosition: 'bottom' | 'side'; + splitPanelOpen: boolean; + splitPanelHeaderHeight: number; + splitPanelFullHeight: number; +} + +export function computeSplitPanelOffsets({ + hasSplitPanel, + splitPanelPosition, + placement, + splitPanelOpen, + splitPanelFullHeight, + splitPanelHeaderHeight, +}: SplitPanelOffsetInput) { + if (!hasSplitPanel || splitPanelPosition !== 'bottom') { + return { + stickyVerticalBottomOffset: placement.insetBlockEnd, + mainContentPaddingBlockEnd: undefined, + }; + } + const mainContentBottomOffset = splitPanelOpen ? splitPanelFullHeight : splitPanelHeaderHeight; + return { + stickyVerticalBottomOffset: mainContentBottomOffset + placement.insetBlockEnd, + mainContentPaddingBlockEnd: mainContentBottomOffset, + }; +} + export function getDrawerStyles( verticalOffsets: VerticalLayoutOutput, isMobile: boolean, diff --git a/src/app-layout/visual-refresh-toolbar/index.tsx b/src/app-layout/visual-refresh-toolbar/index.tsx index 460f783d94..906d590d18 100644 --- a/src/app-layout/visual-refresh-toolbar/index.tsx +++ b/src/app-layout/visual-refresh-toolbar/index.tsx @@ -21,7 +21,12 @@ import { MIN_DRAWER_SIZE, OnChangeParams, useDrawers } from '../utils/use-drawer import { useFocusControl, useMultipleFocusControl } from '../utils/use-focus-control'; import { useSplitPanelFocusControl } from '../utils/use-split-panel-focus-control'; import { ActiveDrawersContext } from '../utils/visibility-context'; -import { computeHorizontalLayout, computeVerticalLayout, CONTENT_PADDING } from './compute-layout'; +import { + computeHorizontalLayout, + computeSplitPanelOffsets, + computeVerticalLayout, + CONTENT_PADDING, +} from './compute-layout'; import { AppLayoutVisibilityContext } from './contexts'; import { AppLayoutInternals } from './interfaces'; import { @@ -461,6 +466,15 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef {/* Rendering a hidden copy of breadcrumbs to trigger their deduplication */} @@ -469,16 +483,11 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef Date: Wed, 29 Jan 2025 07:28:14 +0100 Subject: [PATCH 2/7] chore: Add workflow dispatch to release workflow to allow e2e tests to trigger action (#3226) --- .github/workflows/release.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 08ec8f7d41..8f5f8b8e6a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -3,6 +3,16 @@ name: Release on: + workflow_dispatch: + inputs: + role-to-assume: + type: string + description: Use to override the AWS role used during release + required: false + project-name: + type: string + description: Use to override the CodeBuild project called + required: false push: branches: - main @@ -18,3 +28,5 @@ jobs: secrets: inherit with: skip-test: true + role-to-assume: ${{ inputs.role-to-assume }} + project-name: ${{ inputs.project-name }} From 124c9b894363914f12e4047bcbeede5f0f9887ea Mon Sep 17 00:00:00 2001 From: Aleksandra Danilina <29692746+Al-Dani@users.noreply.github.com> Date: Wed, 29 Jan 2025 15:46:00 +0100 Subject: [PATCH 3/7] chore: Fix elements scrolling with elementScrollTo in integration tests (#3138) --- .../__integ__/app-layout-drawers.test.ts | 11 ++++++++++- src/app-layout/__integ__/runtime-drawers.test.ts | 15 +++++++++++++-- src/cards/__integ__/sticky-header.test.ts | 2 +- src/pie-chart/__integ__/pie-chart.test.ts | 2 -- src/select/__integ__/select.test.ts | 2 -- 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/app-layout/__integ__/app-layout-drawers.test.ts b/src/app-layout/__integ__/app-layout-drawers.test.ts index 806d2594c4..4b7fcfd585 100644 --- a/src/app-layout/__integ__/app-layout-drawers.test.ts +++ b/src/app-layout/__integ__/app-layout-drawers.test.ts @@ -9,6 +9,8 @@ import { viewports } from './constants'; import { testIf } from './utils'; const wrapper = createWrapper().findAppLayout(); +import vrDrawerStyles from '../../../lib/components/app-layout/visual-refresh/styles.selectors.js'; +import vrToolbarDrawerStyles from '../../../lib/components/app-layout/visual-refresh-toolbar/drawer/styles.selectors.js'; class AppLayoutDrawersPage extends BasePageObject { async openFirstDrawer() { @@ -242,7 +244,14 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as const)('%s', theme => setupTest({ theme }, async page => { await page.openFirstDrawer(); const resizeHandleBefore = await page.getResizeHandlePosition(); - await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 }); + const scrollableContainer = + theme === 'classic' + ? wrapper.findActiveDrawer().toSelector() + : theme === 'refresh' + ? `.${vrDrawerStyles['drawer-content-container']}` + : `.${vrToolbarDrawerStyles['drawer-content-container']}`; + + await page.elementScrollTo(scrollableContainer, { top: 100 }); const resizeHandleAfter = await page.getResizeHandlePosition(); await expect(resizeHandleAfter).toEqual(resizeHandleBefore); }) diff --git a/src/app-layout/__integ__/runtime-drawers.test.ts b/src/app-layout/__integ__/runtime-drawers.test.ts index f2d8baa81f..231e05cb8b 100644 --- a/src/app-layout/__integ__/runtime-drawers.test.ts +++ b/src/app-layout/__integ__/runtime-drawers.test.ts @@ -7,6 +7,9 @@ import createWrapper, { AppLayoutWrapper } from '../../../lib/components/test-ut import { viewports } from './constants'; import { getUrlParams, Theme } from './utils'; +import vrDrawerStyles from '../../../lib/components/app-layout/visual-refresh/styles.selectors.js'; +import vrToolbarDrawerStyles from '../../../lib/components/app-layout/visual-refresh-toolbar/drawer/styles.selectors.js'; + const wrapper = createWrapper().findAppLayout(); const findDrawerById = (wrapper: AppLayoutWrapper, id: string) => { return wrapper.find(`[data-testid="awsui-app-layout-drawer-${id}"]`); @@ -117,7 +120,14 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme const getScrollPosition = () => page.getBoundingBox('[data-testid="drawer-sticky-header"]'); const scrollBefore = await getScrollPosition(); - await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 }); + const scrollableContainer = + theme === 'classic' + ? wrapper.findActiveDrawer().toSelector() + : theme === 'refresh' + ? `.${vrDrawerStyles['drawer-content-container']}` + : `.${vrToolbarDrawerStyles['drawer-content-container']}`; + + await page.elementScrollTo(scrollableContainer, { top: 100 }); await expect(getScrollPosition()).resolves.toEqual(scrollBefore); await expect(page.isDisplayed('[data-testid="drawer-sticky-header"]')).resolves.toBe(true); }) @@ -316,7 +326,8 @@ describe('Visual refresh toolbar only', () => { const getScrollPosition = () => page.getBoundingBox('[data-testid="drawer-sticky-header"]'); const scrollBefore = await getScrollPosition(); - await page.elementScrollTo(wrapper.findActiveDrawer().toSelector(), { top: 100 }); + const scrollableContainer = `.${vrToolbarDrawerStyles['drawer-content-container']}`; + await page.elementScrollTo(scrollableContainer, { top: 100 }); await expect(getScrollPosition()).resolves.toEqual(scrollBefore); await expect(page.isDisplayed('[data-testid="drawer-sticky-header"]')).resolves.toBe(true); }) diff --git a/src/cards/__integ__/sticky-header.test.ts b/src/cards/__integ__/sticky-header.test.ts index 0d1dc28417..0e9b89ac50 100644 --- a/src/cards/__integ__/sticky-header.test.ts +++ b/src/cards/__integ__/sticky-header.test.ts @@ -44,7 +44,7 @@ describe('Cards Sticky Header', () => { const scrollTopToBtn = '#scroll-to-top-btn'; const toggleStickinessBtn = '#toggle-stickiness-btn'; const toggleVerticalOffsetBtn = '#toggle-vertical-offset-btn'; - const overflowParentPageHeight = 300; + const overflowParentPageHeight = 400; const verticalOffset = 50; test( diff --git a/src/pie-chart/__integ__/pie-chart.test.ts b/src/pie-chart/__integ__/pie-chart.test.ts index 423775eccd..8906b30cff 100644 --- a/src/pie-chart/__integ__/pie-chart.test.ts +++ b/src/pie-chart/__integ__/pie-chart.test.ts @@ -209,7 +209,6 @@ describe('Legend', () => { 'can be controlled with mouse', setupTest(async page => { // Hover over second segment in the legend - await page.elementScrollTo(legendWrapper.findItems().get(2).toSelector(), { top: 0, left: 0 }); await page.hoverElement(legendWrapper.findItems().get(2).toSelector()); await expect(page.getText(legendWrapper.findHighlightedItem().toSelector())).resolves.toBe('Chocolate'); @@ -239,7 +238,6 @@ describe('Legend', () => { 'highlighted legend elements should be not be highlighted when user hovers away', setupTest(async page => { // Hover over second segment in the legend - await page.elementScrollTo(legendWrapper.findItems().get(2).toSelector(), { top: 0, left: 0 }); await page.hoverElement(legendWrapper.findItems().get(2).toSelector()); // Verify that no legend is highlighted diff --git a/src/select/__integ__/select.test.ts b/src/select/__integ__/select.test.ts index d06101bf00..09e5a48763 100644 --- a/src/select/__integ__/select.test.ts +++ b/src/select/__integ__/select.test.ts @@ -109,8 +109,6 @@ test( const { height: actualDropdownHeight } = await page.getBoundingBox(optionsSelector); const availableDropdownHeight = smallestContainerHeight - triggerHeight; expect(actualDropdownHeight).toBeLessThan(availableDropdownHeight); - const { top: containerScrollTop } = await page.getElementScroll('#smallest_container'); - expect(containerScrollTop).toBe(0); }) ); From bcb11cb9e53a9bae9f6085fd100d68accc3a2d3f Mon Sep 17 00:00:00 2001 From: Avinash Dwarapu Date: Thu, 30 Jan 2025 11:27:36 +0100 Subject: [PATCH 4/7] fix: Correctly overlap sticky columns over static columns in borderless tables (#3231) --- pages/table/sticky-columns.page.tsx | 21 ++++++++++++++++++++- pages/table/styles.scss | 9 +++++++++ src/table/header-cell/styles.scss | 4 ++-- 3 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 pages/table/styles.scss diff --git a/pages/table/sticky-columns.page.tsx b/pages/table/sticky-columns.page.tsx index f0f5fe9aa7..c94d7e149e 100644 --- a/pages/table/sticky-columns.page.tsx +++ b/pages/table/sticky-columns.page.tsx @@ -16,6 +16,8 @@ import ScreenshotArea from '../utils/screenshot-area'; import { generateItems, Instance } from './generate-data'; import { columnsConfig } from './shared-configs'; +import styles from './styles.scss'; + type DemoContext = React.Context< AppContextType<{ loading: boolean; @@ -125,7 +127,7 @@ const COLUMN_DEFINITIONS: TableProps.ColumnDefinition[] = [ { id: 'description-10', header: 'Description', - cell: item => Link: {item.description} || '-', + cell: item => Link: {item.description || '-'}, sortingField: 'description', }, { @@ -340,6 +342,23 @@ export default () => { ariaLabels={{ ...ariaLabels, tableLabel: 'Inline editing table' }} header={
Large table with inline editing
} /> +
+ setSelectedItems(selectedItems)} + items={items} + /> + ); diff --git a/pages/table/styles.scss b/pages/table/styles.scss new file mode 100644 index 0000000000..225bad3063 --- /dev/null +++ b/pages/table/styles.scss @@ -0,0 +1,9 @@ +/* + Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + SPDX-License-Identifier: Apache-2.0 +*/ +@use '~design-tokens' as tokens; + +.borderless-wrapper { + background-color: tokens.$color-background-status-warning; +} diff --git a/src/table/header-cell/styles.scss b/src/table/header-cell/styles.scss index 30fc02bd04..6265f98321 100644 --- a/src/table/header-cell/styles.scss +++ b/src/table/header-cell/styles.scss @@ -75,8 +75,8 @@ $cell-horizontal-padding: awsui.$space-scaled-l; &-variant-full-page.header-cell-hidden { border-block-end-color: transparent; } - &-variant-embedded.is-visual-refresh:not(.header-cell-sticky), - &-variant-borderless.is-visual-refresh:not(.header-cell-sticky) { + &-variant-embedded.is-visual-refresh:not(:is(.header-cell-sticky, .sticky-cell)), + &-variant-borderless.is-visual-refresh:not(:is(.header-cell-sticky, .sticky-cell)) { background: none; } &:last-child, From 9a18a8c003306b1f6217fbfe84f4471e51994c16 Mon Sep 17 00:00:00 2001 From: Boris Serdiuk Date: Thu, 30 Jan 2025 12:53:03 +0100 Subject: [PATCH 5/7] chore: Remove old flashbar metrics (#3234) --- .../__tests__/analytics-metadata.test.tsx | 13 ++ src/flashbar/__tests__/analytics.test.tsx | 185 ------------------ src/flashbar/collapsible-flashbar.tsx | 2 - src/flashbar/flash.tsx | 9 +- src/flashbar/index.tsx | 9 +- src/flashbar/internal/analytics.ts | 35 ---- 6 files changed, 15 insertions(+), 238 deletions(-) delete mode 100644 src/flashbar/__tests__/analytics.test.tsx delete mode 100644 src/flashbar/internal/analytics.ts diff --git a/src/flashbar/__tests__/analytics-metadata.test.tsx b/src/flashbar/__tests__/analytics-metadata.test.tsx index e995e8def7..16a89453c3 100644 --- a/src/flashbar/__tests__/analytics-metadata.test.tsx +++ b/src/flashbar/__tests__/analytics-metadata.test.tsx @@ -11,6 +11,7 @@ import { getGeneratedAnalyticsMetadata } from '@cloudscape-design/component-tool import Button from '../../../lib/components/button'; import Flashbar, { FlashbarProps } from '../../../lib/components/flashbar'; +import { DATA_ATTR_ANALYTICS_FLASHBAR } from '../../../lib/components/internal/analytics/selectors'; import createWrapper from '../../../lib/components/test-utils/dom'; import { validateComponentNameAndLabels } from '../../internal/__tests__/analytics-metadata-test-utils'; @@ -204,4 +205,16 @@ describe('Flashbar renders correct analytics metadata', () => { ...getMetadata(undefined, true, true), }); }); + + describe('analytics attributes', () => { + test(`adds ${DATA_ATTR_ANALYTICS_FLASHBAR} attribute with the flashbar type`, () => { + const wrapper = renderFlashbar({ items: [{ id: '0', type: 'success' }] }); + expect(wrapper.find(`[${DATA_ATTR_ANALYTICS_FLASHBAR}="success"]`)!.getElement()).toBeInTheDocument(); + }); + + test(`adds ${DATA_ATTR_ANALYTICS_FLASHBAR} attribute with the effective flashbar type when loading`, () => { + const wrapper = renderFlashbar({ items: [{ id: '0', type: 'success', loading: true }] }); + expect(wrapper.find(`[${DATA_ATTR_ANALYTICS_FLASHBAR}="info"]`)!.getElement()).toBeInTheDocument(); + }); + }); }); diff --git a/src/flashbar/__tests__/analytics.test.tsx b/src/flashbar/__tests__/analytics.test.tsx deleted file mode 100644 index b32fdeef7e..0000000000 --- a/src/flashbar/__tests__/analytics.test.tsx +++ /dev/null @@ -1,185 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 -import React from 'react'; -import { render as reactRender } from '@testing-library/react'; - -import { clearOneTimeMetricsCache } from '@cloudscape-design/component-toolkit/internal/testing'; - -import Flashbar, { FlashbarProps } from '../../../lib/components/flashbar'; -import { DATA_ATTR_ANALYTICS_FLASHBAR } from '../../../lib/components/internal/analytics/selectors'; -import { createFlashbarWrapper } from './common'; - -declare global { - interface Window { - panorama?: any; - } -} - -const toggleButtonSelector = 'button'; - -function findFlashbarMetric() { - return jest.mocked(window.panorama).mock.calls.filter((args: any) => args[1].eventContext === 'csa_flashbar'); -} - -describe('Analytics', () => { - beforeEach(() => { - window.panorama = () => {}; - jest.spyOn(window, 'panorama'); - }); - afterEach(() => { - clearOneTimeMetricsCache(); - }); - - it('does not send a metric when an empty array is provided', () => { - createFlashbarWrapper(); - expect(findFlashbarMetric()).toHaveLength(0); - }); - - it('sends a render metric when items are provided', () => { - createFlashbarWrapper( - - ); - - expect(findFlashbarMetric()).toEqual([ - [ - 'trackCustomEvent', - expect.objectContaining({ - eventContext: 'csa_flashbar', - eventType: 'render', - eventValue: '2', - eventDetail: expect.any(String), - timestamp: expect.any(Number), - }), - ], - ]); - }); - - it('sends a render metric when stacked items are provided', () => { - createFlashbarWrapper( - - ); - - expect(findFlashbarMetric()).toEqual([ - [ - 'trackCustomEvent', - expect.objectContaining({ - eventContext: 'csa_flashbar', - eventType: 'render', - eventValue: '2', - eventDetail: expect.any(String), - timestamp: expect.any(Number), - }), - ], - ]); - }); - - it('does not send duplicate render metrics on multiple renders', () => { - const items: FlashbarProps['items'] = [ - { type: 'error', header: 'Error', content: 'There was an error' }, - { type: 'success', header: 'Success', content: 'Everything went fine' }, - ]; - - const { rerender } = reactRender(); - jest.mocked(window.panorama).mockClear(); - rerender(); - expect(window.panorama).toBeCalledTimes(0); - }); - - it('sends an expand metric when collapsed', () => { - const wrapper = createFlashbarWrapper( - - ); - jest.mocked(window.panorama).mockClear(); - - wrapper.find(toggleButtonSelector)!.click(); - - expect(window.panorama).toBeCalledTimes(1); - expect(window.panorama).toHaveBeenCalledWith( - 'trackCustomEvent', - expect.objectContaining({ - eventContext: 'csa_flashbar', - eventType: 'expand', - eventValue: '2', - timestamp: expect.any(Number), - }) - ); - }); - - it('sends a collapse metric when collapsed', () => { - const wrapper = createFlashbarWrapper( - - ); - wrapper.find(toggleButtonSelector)!.click(); // expand - jest.mocked(window.panorama).mockClear(); // clear previous events - - wrapper.find(toggleButtonSelector)!.click(); // collapse - expect(window.panorama).toBeCalledTimes(1); - expect(window.panorama).toHaveBeenCalledWith( - 'trackCustomEvent', - expect.objectContaining({ - eventContext: 'csa_flashbar', - eventType: 'collapse', - eventValue: '2', - timestamp: expect.any(Number), - }) - ); - }); - - it('sends a dismiss metric when a flash item is dismissed', () => { - const wrapper = createFlashbarWrapper( - {} }, - ]} - /> - ); - jest.mocked(window.panorama).mockClear(); // clear render event - wrapper.findItems()[0].findDismissButton()!.click(); - - expect(window.panorama).toBeCalledTimes(1); - expect(window.panorama).toHaveBeenCalledWith( - 'trackCustomEvent', - expect.objectContaining({ - eventContext: 'csa_flashbar', - eventType: 'dismiss', - eventValue: 'error', - timestamp: expect.any(Number), - }) - ); - }); - - describe('analytics', () => { - test(`adds ${DATA_ATTR_ANALYTICS_FLASHBAR} attribute with the flashbar type`, () => { - const { container } = reactRender(); - expect(container.querySelector(`[${DATA_ATTR_ANALYTICS_FLASHBAR}="success"]`)).toBeInTheDocument(); - }); - - test(`adds ${DATA_ATTR_ANALYTICS_FLASHBAR} attribute with the effective flashbar type when loading`, () => { - const { container } = reactRender(); - expect(container.querySelector(`[${DATA_ATTR_ANALYTICS_FLASHBAR}="info"]`)).toBeInTheDocument(); - }); - }); -}); diff --git a/src/flashbar/collapsible-flashbar.tsx b/src/flashbar/collapsible-flashbar.tsx index b3e749e615..68dc1a18b4 100644 --- a/src/flashbar/collapsible-flashbar.tsx +++ b/src/flashbar/collapsible-flashbar.tsx @@ -23,7 +23,6 @@ import { getComponentsAnalyticsMetadata, getItemAnalyticsMetadata } from './anal import { useFlashbar } from './common'; import { Flash, focusFlashById } from './flash'; import { FlashbarProps } from './interfaces'; -import { sendToggleMetric } from './internal/analytics'; import { counterTypes, getFlashTypeCount, getItemColor, getVisibleCollapsedItems, StackableItem } from './utils'; import styles from './styles.css.js'; @@ -84,7 +83,6 @@ export default function CollapsibleFlashbar({ items, ...restProps }: FlashbarPro const animateFlash = !isReducedMotion; function toggleCollapseExpand() { - sendToggleMetric(items.length, !isFlashbarStackExpanded); if (!isReducedMotion) { prepareAnimations(); } diff --git a/src/flashbar/flash.tsx b/src/flashbar/flash.tsx index 3472714f35..f84d20fa26 100644 --- a/src/flashbar/flash.tsx +++ b/src/flashbar/flash.tsx @@ -7,7 +7,6 @@ import { useComponentMetadata, warnOnce } from '@cloudscape-design/component-too import { getAnalyticsMetadataAttribute } from '@cloudscape-design/component-toolkit/internal/analytics-metadata'; import { ActionsWrapper } from '../alert/actions-wrapper'; -import { ButtonProps } from '../button/interfaces'; import { InternalButton } from '../button/internal'; import InternalIcon from '../icon/internal'; import { DATA_ATTR_ANALYTICS_FLASHBAR } from '../internal/analytics/selectors'; @@ -23,7 +22,6 @@ import InternalLiveRegion from '../live-region/internal'; import InternalSpinner from '../spinner/internal'; import { GeneratedAnalyticsMetadataFlashbarDismiss } from './analytics-metadata/interfaces'; import { FlashbarProps } from './interfaces'; -import { sendDismissMetric } from './internal/analytics'; import { FOCUS_THROTTLE_DELAY } from './utils'; import analyticsSelectors from './analytics-metadata/styles.css.js'; @@ -142,11 +140,6 @@ export const Flash = React.forwardRef( const effectiveType = loading ? 'info' : type; - const handleDismiss: ButtonProps['onClick'] = event => { - sendDismissMetric(effectiveType); - onDismiss && onDismiss(event); - }; - const analyticsAttributes = { [DATA_ATTR_ANALYTICS_FLASHBAR]: effectiveType, }; @@ -230,7 +223,7 @@ export const Flash = React.forwardRef( onButtonClick={onButtonClick} /> - {dismissible && dismissButton(dismissLabel, handleDismiss)} + {dismissible && dismissButton(dismissLabel, onDismiss)} {ariaRole === 'status' && ( )} diff --git a/src/flashbar/index.tsx b/src/flashbar/index.tsx index f3095b3728..3bb9183a7b 100644 --- a/src/flashbar/index.tsx +++ b/src/flashbar/index.tsx @@ -1,22 +1,15 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import React, { useEffect } from 'react'; +import React from 'react'; import { applyDisplayName } from '../internal/utils/apply-display-name'; import CollapsibleFlashbar from './collapsible-flashbar'; import { FlashbarProps } from './interfaces'; -import { sendRenderMetric } from './internal/analytics'; import NonCollapsibleFlashbar from './non-collapsible-flashbar'; export { FlashbarProps }; export default function Flashbar(props: FlashbarProps) { - useEffect(() => { - if (props.items.length > 0) { - sendRenderMetric(props.items); - } - }, [props.items]); - if (props.stackItems) { return ; } else { diff --git a/src/flashbar/internal/analytics.ts b/src/flashbar/internal/analytics.ts deleted file mode 100644 index 5a50f2297c..0000000000 --- a/src/flashbar/internal/analytics.ts +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { metrics } from '../../internal/metrics'; -import { FlashbarProps } from '../interfaces'; -import { getFlashTypeCount } from '../utils'; - -const eventContext = 'csa_flashbar'; - -export const sendRenderMetric = (items: FlashbarProps['items']) => { - const countByType = getFlashTypeCount(items); - - metrics.sendPanoramaMetric({ - eventContext, - eventType: 'render', - eventValue: items.length.toString(), - eventDetail: countByType, - }); -}; - -export const sendToggleMetric = (itemsCount: number, expanded: boolean) => { - metrics.sendPanoramaMetric({ - eventContext, - eventType: expanded ? 'expand' : 'collapse', - eventValue: itemsCount.toString(), - }); -}; - -export const sendDismissMetric = (itemType: string) => { - metrics.sendPanoramaMetric({ - eventContext, - eventType: 'dismiss', - eventValue: itemType, - }); -}; From ae3d6be7347c14aa8ce1be8ea773f712c53b6650 Mon Sep 17 00:00:00 2001 From: Boris Serdiuk Date: Thu, 30 Jan 2025 14:01:17 +0100 Subject: [PATCH 6/7] chore: Update metrics reporting (#3236) --- src/alert/__tests__/runtime-content.test.tsx | 4 ++-- src/flashbar/__tests__/runtime-content.test.tsx | 2 +- src/internal/plugins/__tests__/api.test.ts | 2 +- src/internal/plugins/controllers/__tests__/drawers.test.ts | 4 ++-- src/internal/plugins/helpers/metrics.ts | 4 ++-- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/alert/__tests__/runtime-content.test.tsx b/src/alert/__tests__/runtime-content.test.tsx index aedae4a5d5..55bcd44ad4 100644 --- a/src/alert/__tests__/runtime-content.test.tsx +++ b/src/alert/__tests__/runtime-content.test.tsx @@ -305,7 +305,7 @@ describe('asynchronous rendering', () => { const message = `"${method}" called after component unmounted`; expect(consoleWarnSpy).toBeCalledWith('[AwsUi]', '[alert-content-replacer]', message); expect(sendPanoramaMetricSpy).toBeCalledWith({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { component: 'alert-content-replacer', version: expect.any(String), @@ -374,7 +374,7 @@ test('can only register a single provider', () => { ) ); expect(sendPanoramaMetricSpy).toHaveBeenCalledWith({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { component: 'alert-flash-content', version: expect.any(String), diff --git a/src/flashbar/__tests__/runtime-content.test.tsx b/src/flashbar/__tests__/runtime-content.test.tsx index 4402c00c50..27d5896116 100644 --- a/src/flashbar/__tests__/runtime-content.test.tsx +++ b/src/flashbar/__tests__/runtime-content.test.tsx @@ -300,7 +300,7 @@ describe('asynchronous rendering', () => { const message = `"${method}" called after component unmounted`; expect(consoleWarnSpy).toBeCalledWith('[AwsUi]', '[flash-content-replacer]', message); expect(sendPanoramaMetricSpy).toBeCalledWith({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { component: 'flash-content-replacer', version: expect.any(String), diff --git a/src/internal/plugins/__tests__/api.test.ts b/src/internal/plugins/__tests__/api.test.ts index d28c842887..a8d3f89499 100644 --- a/src/internal/plugins/__tests__/api.test.ts +++ b/src/internal/plugins/__tests__/api.test.ts @@ -82,7 +82,7 @@ describe('usage metrics', () => { expect(sendPanoramaMetricSpy).toHaveBeenCalledTimes(1); expect(sendPanoramaMetricSpy).toHaveBeenCalledWith( expect.objectContaining({ - eventName: 'awsui-runtime-api-loaded', + eventContext: 'awsui-runtime-api-loaded', }) ); diff --git a/src/internal/plugins/controllers/__tests__/drawers.test.ts b/src/internal/plugins/controllers/__tests__/drawers.test.ts index ef926fda31..71517cf5d5 100644 --- a/src/internal/plugins/controllers/__tests__/drawers.test.ts +++ b/src/internal/plugins/controllers/__tests__/drawers.test.ts @@ -116,7 +116,7 @@ describe('console warnings', () => { expect.stringMatching(/multiple app layout instances detected/) ); expect(sendPanoramaMetricSpy).toHaveBeenCalledWith({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { component: 'app-layout-drawers', version: expect.any(String), @@ -136,7 +136,7 @@ describe('console warnings', () => { expect.stringMatching(/drawer with id "drawerA" is already registered/) ); expect(sendPanoramaMetricSpy).toHaveBeenCalledWith({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { component: 'app-layout-drawers', version: expect.any(String), diff --git a/src/internal/plugins/helpers/metrics.ts b/src/internal/plugins/helpers/metrics.ts index 53a1aeb6b0..71f253a0b4 100644 --- a/src/internal/plugins/helpers/metrics.ts +++ b/src/internal/plugins/helpers/metrics.ts @@ -4,13 +4,13 @@ import { PACKAGE_VERSION } from '../../environment'; import { metrics } from '../../metrics'; export function reportRuntimeApiLoadMetric() { - metrics.sendPanoramaMetric({ eventName: 'awsui-runtime-api-loaded', eventDetail: { version: PACKAGE_VERSION } }); + metrics.sendPanoramaMetric({ eventContext: 'awsui-runtime-api-loaded', eventDetail: { version: PACKAGE_VERSION } }); } export function reportRuntimeApiWarning(component: string, message: string) { console.warn('[AwsUi]', `[${component}]`, message); metrics.sendPanoramaMetric({ - eventName: 'awsui-runtime-api-warning', + eventContext: 'awsui-runtime-api-warning', eventDetail: { version: PACKAGE_VERSION, component, message }, }); } From 2a72da2875cc1802d4d074aaeb04d24c060cafff Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Thu, 30 Jan 2025 20:21:34 +0100 Subject: [PATCH 7/7] fix: Allows table cells be expandable and editable at the same time (#3205) --- pages/table/editable.page.tsx | 54 +++- src/table/__integ__/inline-editing.test.ts | 289 +++++++++--------- src/table/__tests__/body-cell.test.tsx | 15 - src/table/__tests__/expandable-rows.test.tsx | 5 +- src/table/__tests__/table.test.tsx | 77 +++-- .../body-cell/disabled-inline-editor.tsx | 51 ++-- src/table/body-cell/index.tsx | 29 +- src/table/body-cell/styles.scss | 57 ++-- src/table/body-cell/td-element.tsx | 19 +- src/table/header-cell/index.tsx | 2 +- 10 files changed, 345 insertions(+), 253 deletions(-) diff --git a/pages/table/editable.page.tsx b/pages/table/editable.page.tsx index 5f04fafd6e..b58ccb799d 100644 --- a/pages/table/editable.page.tsx +++ b/pages/table/editable.page.tsx @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import React, { ForwardedRef, forwardRef, useContext, useEffect, useRef, useState } from 'react'; -import { Box, Button, Checkbox, Link, Modal, SpaceBetween } from '~components'; +import { Box, Button, Checkbox, Modal, SpaceBetween } from '~components'; import Alert from '~components/alert'; import Autosuggest, { AutosuggestProps } from '~components/autosuggest'; import Header from '~components/header'; @@ -20,6 +20,7 @@ type PageContext = React.Context< AppContextType<{ resizableColumns: boolean; enableKeyboardNavigation: boolean; + expandableRows: boolean; }> >; @@ -62,7 +63,27 @@ const columns: TableProps.ColumnDefinition[] = [ header: 'Distribution ID', sortingField: 'Id', width: 180, - cell: (item: DistributionInfo) => {item.Id}, + cell: (item: DistributionInfo) => item.Id, + editConfig: { + ariaLabel: 'Distribution ID', + editIconAriaLabel: 'editable', + errorIconAriaLabel: 'Distribution ID Error', + editingCell(item, { currentValue, setValue }: TableProps.CellContext) { + return ( + setValue(event.detail.value))} + /> + ); + }, + disabledReason(item) { + if (item.Id.includes('E2')) { + return "You don't have the necessary permissions to edit this item."; + } + return undefined; + }, + }, }, { id: 'DomainName', @@ -222,7 +243,7 @@ const Demo = forwardRef( ) => { const [items, setItems] = useState(initialItems); const { - urlParams: { resizableColumns = true, enableKeyboardNavigation = false }, + urlParams: { resizableColumns = true, enableKeyboardNavigation = false, expandableRows = false }, } = useContext(AppContext as PageContext); const handleSubmit: TableProps.SubmitEditFunction = async (currentItem, column, newValue) => { @@ -251,6 +272,8 @@ const Demo = forwardRef( setClean(); }; + const [expandedItems, setExpandedItems] = useState([]); + return (
[ + { ...item, Id: item.Id + '-1' }, + { ...item, Id: item.Id + '-2' }, + ], + isItemExpandable: item => !item.Id.endsWith('-1') && !item.Id.endsWith('-2'), + expandedItems, + onExpandableItemToggle: ({ detail }) => { + if (detail.expanded) { + return setExpandedItems(prev => [...prev, detail.item]); + } else { + return setExpandedItems(prev => prev.filter(item => item !== detail.item)); + } + }, + } + : undefined + } /> ); } @@ -280,7 +322,7 @@ const Demo = forwardRef( export default function () { const { - urlParams: { resizableColumns = true, enableKeyboardNavigation = false }, + urlParams: { resizableColumns = true, enableKeyboardNavigation = false, expandableRows = false }, setUrlParams, } = useContext(AppContext as PageContext); const [modalVisible, setModalVisible] = useState(false); @@ -321,6 +363,10 @@ export default function () { > Keyboard navigation + + setUrlParams({ expandableRows: event.detail.checked })}> + Expandable rows + diff --git a/src/table/__integ__/inline-editing.test.ts b/src/table/__integ__/inline-editing.test.ts index e806003391..4cda2e3dd9 100644 --- a/src/table/__integ__/inline-editing.test.ts +++ b/src/table/__integ__/inline-editing.test.ts @@ -1,5 +1,6 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 + import range from 'lodash/range'; import { BasePageObject } from '@cloudscape-design/browser-test-tools/page-objects'; @@ -9,203 +10,217 @@ import createWrapper from '../../../lib/components/test-utils/selectors'; import styles from '../../../lib/components/table/body-cell/styles.selectors.js'; -const DOMAIN_ERROR = 'Must be a valid domain name'; -const tableWrapper = createWrapper().findTable()!; - // $ = selector -const bodyCell = tableWrapper.findBodyCell(2, 2)!; -const cellRoot$ = bodyCell.toSelector(); -const cellInputField$ = bodyCell.findFormField().find('input').toSelector(); -const cellEditButton$ = tableWrapper.findEditCellButton(2, 2).toSelector(); -const cellSaveButton = tableWrapper.findEditingCellSaveButton(); -const successIcon$ = bodyCell.findByClassName(styles['body-cell-success']).toSelector(); -const ariaLiveAnnouncement$ = bodyCell.find(`[aria-live="polite"]`).toSelector(); +const distributionIdRow1 = [1, 1] as const; +const distributionIdRow2 = [2, 1] as const; // Disabled +const domainNameRow1 = [1, 2] as const; +const domainNameRow2 = [2, 2] as const; +const tslVersionRow4 = [4, 5] as const; -// for arrow key navigation -const mainCell = tableWrapper.findBodyCell(4, 5); -const mainCell$ = mainCell.toSelector(); -const leftCell$ = tableWrapper.findEditCellButton(4, 4).toSelector(); -const rightCell$ = tableWrapper.findEditCellButton(4, 6).toSelector(); -const cellAbove$ = tableWrapper.findEditCellButton(3, 5).toSelector(); -const cellBelow$ = tableWrapper.findEditCellButton(5, 5).toSelector(); +const tableWrapper = createWrapper().findTable(); +const cellSaveButton$ = tableWrapper.findEditingCellSaveButton().toSelector(); +const liveRegion$ = createWrapper().findLiveRegion().toSelector(); -const bodyCellError = bodyCell.findFormField().findError().toSelector(); - -const disabledCell = tableWrapper.findBodyCell(4, 4); -const disabledCell$ = disabledCell.toSelector(); -const disabledCellLiveRegion$ = createWrapper().findLiveRegion().toSelector(); +function cell$(rowIndex: number, columnIndex: number) { + return tableWrapper.findBodyCell(rowIndex, columnIndex).toSelector(); +} +function cellEditButton$(rowIndex: number, columnIndex: number) { + return tableWrapper.findEditCellButton(rowIndex, columnIndex).toSelector(); +} +function cellExpandToggle$(rowIndex: number) { + return tableWrapper.findExpandToggle(rowIndex).toSelector(); +} +function cellError$(rowIndex: number, columnIndex: number) { + return tableWrapper.findBodyCell(rowIndex, columnIndex).findFormField().findError().toSelector(); +} +function cellInput$(rowIndex: number, columnIndex: number) { + return tableWrapper.findBodyCell(rowIndex, columnIndex).findFormField().find('input').toSelector(); +} +function cellSuccessIcon$(rowIndex: number, columnIndex: number) { + return tableWrapper.findBodyCell(rowIndex, columnIndex).findByClassName(styles['body-cell-success']).toSelector(); +} interface TestOptions { enableKeyboardNavigation?: boolean; + expandableRows?: boolean; } const setupTest = ( - { enableKeyboardNavigation = false }: TestOptions, + { enableKeyboardNavigation = false, expandableRows = false }: TestOptions, testFn: (page: BasePageObject) => Promise ) => { return useBrowser(async browser => { const page = new BasePageObject(browser); await page.setWindowSize({ width: 1200, height: 800 }); - const query = new URLSearchParams({ enableKeyboardNavigation: String(enableKeyboardNavigation) }); + const query = new URLSearchParams({ + enableKeyboardNavigation: String(enableKeyboardNavigation), + expandableRows: String(expandableRows), + }); await browser.url(`#/light/table/editable?${query.toString()}`); await testFn(page); }); }; test( - 'input field is displayed when editable cell is clicked', - setupTest({}, async page => { - const value = await page.getText(cellRoot$); - await page.click(cellRoot$); - await expect(page.getValue(cellInputField$)).resolves.toBe(value); + 'input field is displayed when click on editable cell', + setupTest({ expandableRows: false }, async page => { + const value = await page.getText(cell$(...distributionIdRow1)); + await expect(page.isDisplayed(cellExpandToggle$(distributionIdRow1[0]))).resolves.toBe(false); + await page.click(cell$(...distributionIdRow1)); + await expect(page.getValue(cellInput$(...distributionIdRow1))).resolves.toBe(value); + await expect(page.isDisplayed(cellSaveButton$)).resolves.toBe(true); }) ); test( - 'errorText is displayed when input field is invalid', - setupTest({}, async page => { - await page.click(cellRoot$); - await page.setValue(cellInputField$, 'xyz .com'); // space is not allowed - await expect(page.getText(bodyCellError)).resolves.toBe(DOMAIN_ERROR); + 'input field is displayed when click on cell edit button inside expandable column', + setupTest({ expandableRows: true }, async page => { + const value = await page.getText(cell$(...distributionIdRow1)); + await expect(page.isDisplayed(cellExpandToggle$(distributionIdRow1[0]))).resolves.toBe(true); + await page.click(cellEditButton$(...distributionIdRow1)); + await expect(page.getValue(cellInput$(...distributionIdRow1))).resolves.toBe(value); + await expect(page.isDisplayed(cellSaveButton$)).resolves.toBe(true); }) ); -test.each([false, true])( - 'after edit is submitted, cell is focused, success icon is displayed and aria live region is rendered [enableKeyboardNavigation=%s]', - enableKeyboardNavigation => { - setupTest({ enableKeyboardNavigation }, async page => { - await page.click(cellRoot$); - await page.click(cellSaveButton.toSelector()); +test( + 'disabled reason is displayed when click on disabled editable cell', + setupTest({ expandableRows: false }, async page => { + await expect(page.isDisplayed(cellExpandToggle$(distributionIdRow1[0]))).resolves.toBe(false); - await expect(page.isFocused(cellEditButton$)).resolves.toBe(true); - await expect(page.isDisplayed(successIcon$)).resolves.toBe(true); - await expect(page.getElementProperty(ariaLiveAnnouncement$, 'textContent')).resolves.toBe('Edit successful'); - }); - } + // Click on cell with disabled inline edit + await page.click(cell$(...distributionIdRow2)); + await expect(page.getText(liveRegion$)).resolves.toContain("You don't have the necessary permissions"); + + // Dismiss with click outside + await page.click('[data-testid="focus"]'); + await expect(page.getElementsCount(liveRegion$)).resolves.toBe(0); + }) ); test( - 'can start editing with mouse', - setupTest({}, async page => { - await page.click(cellEditButton$); - await expect(page.isDisplayed(cellSaveButton.toSelector())).resolves.toBe(true); + 'disabled reason is displayed when click on disabled edit button inside expandable column', + setupTest({ expandableRows: true }, async page => { + await expect(page.isDisplayed(cellExpandToggle$(distributionIdRow1[0]))).resolves.toBe(true); + + // Click on cell with disabled inline edit + await page.click(cellEditButton$(...distributionIdRow2)); + await expect(page.getText(liveRegion$)).resolves.toContain("You don't have the necessary permissions"); + + // Dismiss with click outside + await page.click('[data-testid="focus"]'); + await expect(page.getElementsCount(liveRegion$)).resolves.toBe(0); }) ); -test.each(['Enter', 'Space'])('can start editing with %s key', key => { +test( + 'errorText is displayed when input field is invalid', setupTest({}, async page => { - // Focus element before the table - await page.click('[data-testid="focus"]'); - - // Tab to the first editable column - await page.keys(range(11).map(() => 'Tab')); - await expect(page.isFocused(tableWrapper.findEditCellButton(1, 2).toSelector())).resolves.toBe(true); - - // Activate with given key - await page.keys([key]); - await expect(page.isDisplayed(cellSaveButton.toSelector())).resolves.toBe(true); - }); -}); + await page.click(cell$(...domainNameRow2)); + await page.setValue(cellInput$(...domainNameRow2), 'xyz .com'); // space is not allowed + await expect(page.getText(cellError$(...domainNameRow2))).resolves.toBe('Must be a valid domain name'); + await expect(page.isDisplayed(cellSaveButton$)).resolves.toBe(true); + }) +); -test.each(['Enter', 'Space'])('can start editing with %s key with keyboard navigation', key => { +test( + 'click focusable element outside when editing cancels editing and focuses clicked element', setupTest({}, async page => { - // Focus element before the table - await page.click('[data-testid="focus"]'); + // Edit a cell + await page.click(cellEditButton$(...domainNameRow2)); + await expect(page.isFocused(cellInput$(...domainNameRow2))).resolves.toBe(true); - // Tab to the first cell - await page.keys(['Tab', 'Tab']); + // Click on the input element outside, it should get focused. + await page.click('[data-testid="focus"]'); + await expect(page.isFocused('[data-testid="focus"]')).resolves.toBe(true); + }) +); - // Navigate to the first editable column - await page.keys(['ArrowDown', 'ArrowRight']); - await expect(page.isFocused(tableWrapper.findEditCellButton(1, 2).toSelector())).resolves.toBe(true); +describe.each([false, true])('enableKeyboardNavigation=%s', enableKeyboardNavigation => { + test( + 'after edit is submitted, cell is focused, success icon is displayed and aria live region is rendered [enableKeyboardNavigation=%s]', + setupTest({ enableKeyboardNavigation }, async page => { + await page.click(cell$(...domainNameRow2)); + await page.click(cellSaveButton$); + + await expect(page.isFocused(cellEditButton$(...domainNameRow2))).resolves.toBe(true); + await expect(page.isDisplayed(cellSuccessIcon$(...domainNameRow2))).resolves.toBe(true); + await expect(page.getElementProperty(liveRegion$, 'textContent')).resolves.toBe('Edit successful'); + }) + ); + + test.each([ + { expandableRows: false, key: 'Enter' }, + { expandableRows: false, key: 'Space' }, + { expandableRows: true, key: 'Enter' }, + { expandableRows: true, key: 'Space' }, + ])('can start editing with $key key, expandableRows=$expandableRows', async ({ expandableRows, key }) => { + await setupTest({ enableKeyboardNavigation, expandableRows }, async page => { + // Focus element before the table + await page.click('[data-testid="focus"]'); - // Activate with given key - await page.keys([key]); - await expect(page.isDisplayed(cellSaveButton.toSelector())).resolves.toBe(true); + // Navigate to the target cell + if (enableKeyboardNavigation) { + await page.keys(['Tab', 'Tab']); + await page.keys(['ArrowDown', 'ArrowRight']); + } else { + await page.keys(range(11).map(() => 'Tab')); + } + const targetRow = (expandableRows ? distributionIdRow1 : domainNameRow1) as [number, number]; + await expect(page.isFocused(cellEditButton$(...targetRow))).resolves.toBe(true); + + // Activate with given key + await page.keys([key]); + await expect(page.isDisplayed(cellSaveButton$)).resolves.toBe(true); + })(); }); -}); -test.each([false, true])( - 'cell focus is moved when arrow keys are pressed [enableKeyboardNavigation=%s]', - enableKeyboardNavigation => { + test( + 'cell focus is moved when arrow keys are pressed', setupTest({ enableKeyboardNavigation }, async page => { - await page.click(mainCell$); - await page.click(cellSaveButton.toSelector()); + await page.click(cell$(...tslVersionRow4)); + await page.click(cellSaveButton$); await page.keys(['ArrowRight']); - await expect(page.isFocused(rightCell$)).resolves.toBe(true); + await expect(page.isFocused(cellEditButton$(tslVersionRow4[0], tslVersionRow4[1] + 1))).resolves.toBe(true); await page.keys(['ArrowLeft', 'ArrowLeft']); - await expect(page.isFocused(leftCell$)).resolves.toBe(true); + await expect(page.isFocused(cellEditButton$(tslVersionRow4[0], tslVersionRow4[1] - 1))).resolves.toBe(true); await page.keys(['ArrowRight', 'ArrowUp']); - await expect(page.isFocused(cellAbove$)).resolves.toBe(true); + await expect(page.isFocused(cellEditButton$(tslVersionRow4[0] - 1, tslVersionRow4[1]))).resolves.toBe(true); await page.keys(['ArrowDown', 'ArrowDown']); - await expect(page.isFocused(cellBelow$)).resolves.toBe(true); - }); - } -); + await expect(page.isFocused(cellEditButton$(tslVersionRow4[0] + 1, tslVersionRow4[1]))).resolves.toBe(true); + }) + ); -test.each([false, true])( - 'input is focused when the edit operation failed [enableKeyboardNavigation=%s]', - enableKeyboardNavigation => { + test( + 'input is focused when the edit operation failed', setupTest({ enableKeyboardNavigation }, async page => { - await page.click(cellRoot$); + await page.click(cell$(...domainNameRow2)); // "inline" is a special keyword that causes a server-side error - await page.setValue(cellInputField$, 'inline'); + await page.setValue(cellInput$(...domainNameRow2), 'inline'); await page.keys('Enter'); // after loading, the focus should be back on the input - await page.waitForAssertion(() => expect(page.isFocused(cellInputField$)).resolves.toBe(true)); - }); - } -); + await page.waitForAssertion(() => expect(page.isFocused(cellInput$(...domainNameRow2))).resolves.toBe(true)); + }) + ); -test.each([false, true])( - 'click focusable element outside when editing cancels editing and focuses clicked element [enableKeyboardNavigation=%s]', - enableKeyboardNavigation => { - setupTest({ enableKeyboardNavigation }, async page => { - // Edit a cell - await page.click(cellEditButton$); - await expect(page.isFocused(cellInputField$)).resolves.toBe(true); - - // Click on the input element outside, it should get focused. - await page.click('[data-testid="focus"]'); - await expect(page.isFocused('[data-testid="focus"]')).resolves.toBe(true); - }); - } -); - -test( - 'can activate and dismiss disabled reason popover with mouse', - setupTest({}, async page => { - // Click on cell with disabled inline edit - await page.click(disabledCell$); - - await expect(page.getText(disabledCellLiveRegion$)).resolves.toContain( - "You don't have the necessary permissions to change a BrowserStack origin." - ); - - // Dismiss with Escape - await page.keys(['Escape']); - await expect(page.getElementsCount(disabledCellLiveRegion$)).resolves.toBe(0); - }) -); - -test.each([false, true])( - 'can activate disabled reason popover with keyboard [enableKeyboardNavigation=%s]', - enableKeyboardNavigation => { + test( + 'can activate disabled reason popover with keyboard', setupTest({ enableKeyboardNavigation }, async page => { // Navigate to a disabled cell - await page.click(mainCell$); - await page.click(cellSaveButton.toSelector()); + await page.click(cell$(...tslVersionRow4)); + await page.click(cellSaveButton$); await page.keys(['ArrowLeft']); // Activate the popover with Enter await page.keys(['Enter']); + await expect(page.getText(liveRegion$)).resolves.toContain("You don't have the necessary permissions"); - await expect(page.getText(disabledCellLiveRegion$)).resolves.toContain( - "You don't have the necessary permissions to change a BrowserStack origin." - ); - }); - } -); + // Dismiss popover + await page.keys(['Escape']); + await expect(page.getElementsCount(liveRegion$)).resolves.toBe(0); + }) + ); +}); diff --git a/src/table/__tests__/body-cell.test.tsx b/src/table/__tests__/body-cell.test.tsx index 574e417896..4527bfc45f 100644 --- a/src/table/__tests__/body-cell.test.tsx +++ b/src/table/__tests__/body-cell.test.tsx @@ -332,21 +332,6 @@ describe('TableBodyCell', () => { fireEvent.keyDown(disabledButton, { key: 'Escape' }); expect(onEditEndMock).toBeCalledWith(true); }); - - test('show and hide lock icon based on hover when popover is not visible', () => { - const { container } = render(); - - // No icon by default - expect(wrapper(container).findIcon()).toBeNull(); - - // Hover over TD element - fireEvent.mouseEnter(container.querySelector('[data-inline-editing-active]')!); - expect(wrapper(container).findIcon()).not.toBeNull(); - - // Remove mouse - fireEvent.mouseLeave(container.querySelector('[data-inline-editing-active]')!); - expect(wrapper(container).findIcon()).toBeNull(); - }); }); test('does not set tab index when negative', () => { diff --git a/src/table/__tests__/expandable-rows.test.tsx b/src/table/__tests__/expandable-rows.test.tsx index e3356c4207..528a3acc25 100644 --- a/src/table/__tests__/expandable-rows.test.tsx +++ b/src/table/__tests__/expandable-rows.test.tsx @@ -283,7 +283,7 @@ describe('Expandable rows', () => { expect(table.findEditCellButton(1, 2)).not.toBe(null); }); - test('first column of expandable table cannot be editable', () => { + test('columns can be both editable and expandable', () => { const { table } = renderTable({ items: simpleItems, columnDefinitions: [ @@ -297,7 +297,6 @@ describe('Expandable rows', () => { onExpandableItemToggle: () => {}, }, }); - expect(table.findEditCellButton(1, 1)).toBe(null); - expect(table.findEditCellButton(1, 2)).not.toBe(null); + expect(table.findEditCellButton(1, 1)).not.toBe(null); }); }); diff --git a/src/table/__tests__/table.test.tsx b/src/table/__tests__/table.test.tsx index 0b632ccef9..1fe5be7867 100644 --- a/src/table/__tests__/table.test.tsx +++ b/src/table/__tests__/table.test.tsx @@ -9,6 +9,7 @@ import Select from '../../../lib/components/select'; import Table, { TableProps } from '../../../lib/components/table'; import createWrapper, { ElementWrapper, PaginationWrapper, TableWrapper } from '../../../lib/components/test-utils/dom'; +import popoverStyles from '../../../lib/components/popover/styles.css.js'; import bodyCellStyles from '../../../lib/components/table/body-cell/styles.css.js'; import headerCellStyles from '../../../lib/components/table/header-cell/styles.css.js'; import styles from '../../../lib/components/table/styles.css.js'; @@ -228,36 +229,21 @@ test('should render table header with icons to indicate editable columns', () => }); }); -test('should show edit icon on hover', () => { +test('should show success icon after edit is saved', () => { const { wrapper } = renderTable(
); - // No icon by default - const editButton = wrapper.findEditCellButton(1, 1); - expect(editButton?.findIcon()).toBeNull(); + // No success icon by default + expect(wrapper.findBodyCell(1, 1)!.findByClassName(bodyCellStyles['body-cell-success'])).toBe(null); - // Show icon on hover - fireEvent.mouseEnter(editButton!.getElement()); - expect(editButton?.findIcon()).not.toBeNull(); + // Shows no icon if edit was discarded + wrapper.findBodyCell(1, 1)!.click(); + wrapper.findEditingCellCancelButton()!.click(); + expect(wrapper.findBodyCell(1, 1)!.findByClassName(bodyCellStyles['body-cell-success'])).toBe(null); - // Remove icon when mouse moves away - fireEvent.mouseLeave(editButton!.getElement()); - expect(editButton?.findIcon()).toBeNull(); -}); - -test('should show edit icon on focus', () => { - const { wrapper } = renderTable(
); - - // No icon by default - const editButton = wrapper.findEditCellButton(1, 1); - expect(editButton?.findIcon()).toBeNull(); - - // Show icon on focus - editButton?.focus(); - expect(editButton?.findIcon()).not.toBeNull(); - - // Remove icon on blur - editButton?.blur(); - expect(editButton?.findIcon()).toBeNull(); + // Shows success icon if edit completes successfully + wrapper.findBodyCell(1, 1)!.click(); + wrapper.findEditingCellSaveButton()!.click(); + expect(wrapper.findBodyCell(1, 1)!.findByClassName(bodyCellStyles['body-cell-success'])).not.toBe(null); }); test('should cancel edit using ref imperative method', async () => { @@ -524,3 +510,42 @@ test('should submit edits successfully', async () => { expect(data.name).toBe('banana'); }); }); + +test('shows and hides cell disabled reason', () => { + const { wrapper } = renderTable( + + columnDefinitions={[ + { + id: 'name', + header: 'Name', + cell: item => item.name, + editConfig: { + ariaLabel: 'test-name', + constraintText: 'test-constraint', + editingCell: () => null, + disabledReason: item => `Cannot edit ${item.name}`, + }, + }, + ]} + items={[{ name: 'test1' }, { name: 'test2' }]} + ariaLabels={{ + activateEditLabel() { + return 'activate-edit'; + }, + cancelEditLabel() { + return 'cancel-edit'; + }, + submitEditLabel() { + return 'save-edit'; + }, + }} + submitEdit={() => {}} + /> + ); + + wrapper.findEditCellButton(1, 1)!.click(); + expect(createWrapper().findByClassName(popoverStyles.container)!.getElement()).toHaveTextContent('Cannot edit test1'); + + wrapper.findEditCellButton(2, 1)!.click(); + expect(createWrapper().findByClassName(popoverStyles.container)!.getElement()).toHaveTextContent('Cannot edit test2'); +}); diff --git a/src/table/body-cell/disabled-inline-editor.tsx b/src/table/body-cell/disabled-inline-editor.tsx index 15727bdf72..a2cd80c96c 100644 --- a/src/table/body-cell/disabled-inline-editor.tsx +++ b/src/table/body-cell/disabled-inline-editor.tsx @@ -1,6 +1,6 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import React, { useRef, useState } from 'react'; +import React, { useRef } from 'react'; import clsx from 'clsx'; import Icon from '../../icon/internal'; @@ -32,17 +32,13 @@ export function DisabledInlineEditor({ editDisabledReason, ...rest }: DisabledInlineEditorProps) { + const isExpandableColumn = rest.level !== undefined; const clickAwayRef = useClickAway(() => { if (isEditing) { onEditEnd(true); } }); - const [hasHover, setHasHover] = useState(false); - const [hasFocus, setHasFocus] = useState(false); - // When a cell is both expandable and editable the icon is always shown. - const showIcon = hasHover || hasFocus || isEditing; - const iconRef = useRef(null); const buttonRef = useRef(null); const portalRef = useRef(null); @@ -71,29 +67,34 @@ export function DisabledInlineEditor({ } isEditing={isEditing} isEditingDisabled={true} - onClick={!isEditing ? onClick : undefined} - onMouseEnter={() => setHasHover(true)} - onMouseLeave={() => setHasHover(false)} - ref={clickAwayRef} + onClick={!isEditing && !isExpandableColumn ? onClick : undefined} + ref={!isExpandableColumn ? clickAwayRef : undefined} > {column.cell(item)}
- +
+ +
{isEditing && ( diff --git a/src/table/body-cell/index.tsx b/src/table/body-cell/index.tsx index bd86318c38..d45f2c3fdc 100644 --- a/src/table/body-cell/index.tsx +++ b/src/table/body-cell/index.tsx @@ -1,6 +1,7 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 import React, { useEffect, useRef, useState } from 'react'; +import clsx from 'clsx'; import { useInternalI18n } from '../../i18n/context'; import Icon from '../../icon/internal'; @@ -45,6 +46,7 @@ function TableCellEditable({ 'data-inline-editing-active': isEditing.toString(), }; const isFocusMoveNeededRef = useRef(false); + const isExpandableColumn = rest.level !== undefined; useEffect(() => { if (!isEditing && editActivateRef.current && isFocusMoveNeededRef.current) { @@ -53,10 +55,7 @@ function TableCellEditable({ } }, [isEditing]); // To improve the initial page render performance we only show the edit icon when necessary. - const [hasHover, setHasHover] = useState(false); const [hasFocus, setHasFocus] = useState(false); - // When a cell is both expandable and editable the icon is always shown. - const showIcon = hasHover || hasFocus; const prevSuccessfulEdit = usePrevious(successfulEdit); const prevHasFocus = usePrevious(hasFocus); @@ -80,10 +79,10 @@ function TableCellEditable({ {...rest} nativeAttributes={tdNativeAttributes as TableTdElementProps['nativeAttributes']} isEditing={isEditing} - hasSuccessIcon={showSuccessIcon && showIcon} - onClick={!isEditing ? onEditStart : undefined} - onMouseEnter={() => setHasHover(true)} - onMouseLeave={() => setHasHover(false)} + hasSuccessIcon={showSuccessIcon && hasFocus} + onClick={!isEditing && !isExpandableColumn ? onEditStart : undefined} + onFocus={() => setHasFocus(true)} + onBlur={() => setHasFocus(false)} > {isEditing && column.editConfig ? ( ({ <> {column.cell(item)} - {showSuccessIcon && showIcon && ( + {showSuccessIcon && hasFocus && ( <> ({
@@ -140,15 +140,14 @@ function TableCellEditable({ } export function TableBodyCell(props: TableBodyCellProps) { - const isExpandableColumnCell = props.level !== undefined; const editDisabledReason = props.column.editConfig?.disabledReason?.(props.item); // Inline editing is deactivated for expandable column because editable cells are interactive // and cannot include interactive content such as expand toggles. - if (editDisabledReason && !isExpandableColumnCell) { + if (editDisabledReason) { return ; } - if ((props.isEditable || props.isEditing) && !isExpandableColumnCell) { + if (props.isEditable || props.isEditing) { return ; } diff --git a/src/table/body-cell/styles.scss b/src/table/body-cell/styles.scss index f5c121f77a..54ee5cad0c 100644 --- a/src/table/body-cell/styles.scss +++ b/src/table/body-cell/styles.scss @@ -117,7 +117,7 @@ $cell-negative-space-vertical: 2px; } } @mixin body-cell-active-hover-padding($padding-start) { - &:not(.body-cell-edit-active).body-cell-editable:hover { + &:not(.body-cell-edit-active):not(.body-cell-expandable).body-cell-editable:hover { @include cell-padding-inline-start(calc(#{$padding-start} + #{awsui.$border-divider-list-width})); } } @@ -358,8 +358,8 @@ $cell-negative-space-vertical: 2px; background: 0; border-block: 0; border-inline: 0; - padding-block: 0; - padding-inline: 0; + padding-block: awsui.$space-scaled-xxs; + padding-inline: awsui.$space-scaled-xxs; // This gives the editor button a small area even when the icon is not rendered. // That is to allow programmatic interaction in tests. @@ -394,6 +394,16 @@ $cell-negative-space-vertical: 2px; // 6 space-xxs: 2 * icon left padding + 2 * icon right padding + space between icons + space between icons and editor max-inline-size: calc(100% - 6 * #{awsui.$space-xxs} - 2 * #{awsui.$size-icon-normal}); } + + &-focusable { + @include focus-visible.when-visible { + // Making focus outline slightly smaller to not intersect with the success indicator. + @include styles.focus-highlight(-1px); + } + } + } + &-editor-icon { + display: none; } &.body-cell-expandable { @@ -422,8 +432,6 @@ $cell-negative-space-vertical: 2px; } &:not(.body-cell-edit-active) { - cursor: pointer; - // Include interactive padding even when a cell is not hovered to prevent jittering when resizableColumns=false. &:not(.resizable-columns) { @include cell-padding-inline-end($interactive-column-padding-inline-end); @@ -444,17 +452,21 @@ $cell-negative-space-vertical: 2px; opacity: 0; } - // Showing focus outline for the cell. - // We don't use our focus-visible polyfill here because it doesn't work properly with screen readers. - // These edit buttons are special because they are visually hidden (opacity: 0), but exposed to assistive technology. - // It's therefore important to display the focus outline, even when a keyboard use wasn't detected. - // For example, when an edit button is selected from the VoiceOver rotor menu. - &:focus-within { - @include cell-focus-outline; + // The editable cells are interactive but the actual focus lands on the edit button which is decorative. + // That is why we use focus-within to detect if the focus is on the edit button to draw the outline around the cell. + // For expandable+editable cells the edit button works as a normal button because the cell itself is not interactive. + &:not(.body-cell-expandable) { + &:focus-within { + @include cell-focus-outline; + } } &:focus-within:focus-within, &.body-cell-edit-disabled-popover { + // stylelint-disable-next-line @cloudscape-design/no-implicit-descendant, no-descending-specificity + .body-cell-editor-icon { + display: unset; + } &.body-cell-has-success { // After a successful edit, we display the success icon next to the edit button and need additional padding to not let the text overflow the success icon. @include cell-padding-inline-end( @@ -470,10 +482,19 @@ $cell-negative-space-vertical: 2px; &:hover:hover { position: relative; - background-color: awsui.$color-background-dropdown-item-hover; - border-block: awsui.$border-divider-list-width solid awsui.$color-border-editable-cell-hover; - border-inline: awsui.$border-divider-list-width solid awsui.$color-border-editable-cell-hover; - inset-inline: calc(-1 * #{awsui.$border-divider-list-width}); + // stylelint-disable-next-line @cloudscape-design/no-implicit-descendant + .body-cell-editor-icon { + display: unset; + } + + &:not(.body-cell-expandable) { + cursor: pointer; + background-color: awsui.$color-background-dropdown-item-hover; + border-block: awsui.$border-divider-list-width solid awsui.$color-border-editable-cell-hover; + border-inline: awsui.$border-divider-list-width solid awsui.$color-border-editable-cell-hover; + inset-inline: calc(-1 * #{awsui.$border-divider-list-width}); + } + &.sticky-cell { position: sticky; } @@ -504,12 +525,12 @@ $cell-negative-space-vertical: 2px; &.body-cell-next-selected { @include cell-padding-block(calc(#{$cell-vertical-padding} - calc(#{awsui.$border-divider-list-width} / 2))); } - &.body-cell-last-row:not(.body-cell-selected) { + &.body-cell-last-row:not(.body-cell-expandable):not(.body-cell-selected) { @include cell-padding-block-start( calc(#{$cell-vertical-padding} - calc(#{awsui.$border-divider-list-width})) ); } - &.body-cell-first-row:not(.body-cell-selected) { + &.body-cell-first-row:not(.body-cell-expandable):not(.body-cell-selected) { @include cell-padding-block(calc(#{$cell-vertical-padding} - calc(#{awsui.$border-divider-list-width}))); } @include focused-editor-styles; diff --git a/src/table/body-cell/td-element.tsx b/src/table/body-cell/td-element.tsx index 121f5ca8d1..3bdfa8084f 100644 --- a/src/table/body-cell/td-element.tsx +++ b/src/table/body-cell/td-element.tsx @@ -31,8 +31,8 @@ export interface TableTdElementProps { 'style' | 'className' | 'onClick' >; onClick?: () => void; - onMouseEnter?: () => void; - onMouseLeave?: () => void; + onFocus?: () => void; + onBlur?: () => void; children?: React.ReactNode; isEvenRow?: boolean; stripedRows?: boolean; @@ -71,8 +71,8 @@ export const TableTdElement = React.forwardRef(null); const mergedRef = useMergeRefs(stickyStyles.ref, ref, cellRefObject); const { tabIndex: cellTabIndex } = useSingleTabStopNavigation(cellRefObject); + const isEditingActive = isEditing && !isEditingDisabled; return ( - {level !== undefined && isExpandable && ( + {level !== undefined && isExpandable && !isEditingActive && (
({ id={headerId} > {column.header} - {isEditable && !isExpandable ? ( + {isEditable ? (