From 8fa826d2c55c523ceaaee0a8788d1b560291ee86 Mon Sep 17 00:00:00 2001 From: Hannah Mudge Date: Wed, 22 Mar 2023 08:07:40 -0600 Subject: [PATCH] [Dashboard] Fix React errors (#153320) Closes https://github.com/elastic/kibana/issues/150641 ## Summary This PR fixes the following React errors that were being thrown in the Dashboard app: 1. The following React warning was thrown by the `EuiContextMenuPanel` in `ControlsToolbarButton` when the controls button was clicked because the `key` prop was one layer too deep, so it thought that it was missing: ![FirstWarning](https://user-images.githubusercontent.com/8698078/226474063-d43cf58e-1f41-46f1-9d39-2b889735b05d.png) I fixed this by following EUI's guidelines for [pass-through props](https://github.com/elastic/eui/blob/main/wiki/component-design.md#pass-through-props). 2. The following React warning was thrown by the `react-remove-scroll-bar` library when both a flyout and a modal were open at the same time: ![ThirdWarning](https://user-images.githubusercontent.com/8698078/226477555-bbbd3582-e33a-4912-adf4-5f08c609119e.png) I fixed this by upgrading the dependency, since it was fixed in a later version of the library. 3. @elastic/kibana-data-discovery The following React warning was thrown by Discover's saved search embeddable when `totalHitCount` was undefined because the `FormattedNumber` component did not handle it properly: ![SecondWarning](https://user-images.githubusercontent.com/8698078/226475914-1a74c56b-9136-480f-9a9b-3518dd1645b9.png) I fixed this by ensuring that the `TotalDocuments` component was not rendered when `totalHitCount` was undefined. While this was not technically our error to fix, it was simple enough and the error impacted 2 out of the 3 sample dashboards, so I figured I'd throw that fix in as well to clean up our console :+1: I tested a bunch of Dashboard behaviour to try to get other React errors to throw, but I couldn't find any other ones - totally possible I missed something, but I think it's safe to close the attached issue and open separate issues for other React errors (if any) that are discovered after-the-fact. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../control_group/control_group_strings.ts | 4 - .../editor/edit_control_group.tsx | 88 ------------------- .../editor/open_add_data_control_flyout.tsx | 2 + .../editor/open_edit_control_group_flyout.tsx | 62 +++++++++++++ .../embeddable/control_group_container.tsx | 12 +-- src/plugins/controls/tsconfig.json | 1 + .../dashboard_app/_dashboard_app_strings.ts | 5 ++ .../add_data_control_button.tsx | 4 +- .../add_time_slider_control_button.tsx | 4 +- .../controls_toolbar_button.tsx | 19 +++- .../edit_control_group_button.tsx | 34 +++++++ .../public/embeddable/saved_search_grid.tsx | 3 +- .../translations/translations/fr-FR.json | 1 - .../translations/translations/ja-JP.json | 1 - .../translations/translations/zh-CN.json | 1 - yarn.lock | 6 +- 16 files changed, 130 insertions(+), 117 deletions(-) delete mode 100644 src/plugins/controls/public/control_group/editor/edit_control_group.tsx create mode 100644 src/plugins/controls/public/control_group/editor/open_edit_control_group_flyout.tsx create mode 100644 src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/edit_control_group_button.tsx diff --git a/src/plugins/controls/public/control_group/control_group_strings.ts b/src/plugins/controls/public/control_group/control_group_strings.ts index 8492e7f0c47cf..a35baf667dea2 100644 --- a/src/plugins/controls/public/control_group/control_group_strings.ts +++ b/src/plugins/controls/public/control_group/control_group_strings.ts @@ -91,10 +91,6 @@ export const ControlGroupStrings = { i18n.translate('controls.controlGroup.management.addControl', { defaultMessage: 'Add control', }), - getManageButtonTitle: () => - i18n.translate('controls.controlGroup.management.buttonTitle', { - defaultMessage: 'Settings', - }), getFlyoutTitle: () => i18n.translate('controls.controlGroup.management.flyoutTitle', { defaultMessage: 'Control settings', diff --git a/src/plugins/controls/public/control_group/editor/edit_control_group.tsx b/src/plugins/controls/public/control_group/editor/edit_control_group.tsx deleted file mode 100644 index 9cd8531fbc390..0000000000000 --- a/src/plugins/controls/public/control_group/editor/edit_control_group.tsx +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -import React from 'react'; -import { EuiContextMenuItem } from '@elastic/eui'; - -import { toMountPoint } from '@kbn/kibana-react-plugin/public'; -import { OverlayRef } from '@kbn/core/public'; -import { ControlGroupStrings } from '../control_group_strings'; -import { ControlGroupEditor } from './control_group_editor'; -import { pluginServices } from '../../services'; -import { ControlGroupContainer } from '..'; -import { setFlyoutRef } from '../embeddable/control_group_container'; - -export interface EditControlGroupButtonProps { - controlGroupContainer: ControlGroupContainer; - closePopover: () => void; -} - -export const EditControlGroup = ({ - controlGroupContainer, - closePopover, -}: EditControlGroupButtonProps) => { - const { - overlays: { openConfirm, openFlyout }, - theme: { theme$ }, - } = pluginServices.getServices(); - - const editControlGroup = () => { - const onDeleteAll = (ref: OverlayRef) => { - openConfirm(ControlGroupStrings.management.deleteControls.getSubtitle(), { - confirmButtonText: ControlGroupStrings.management.deleteControls.getConfirm(), - cancelButtonText: ControlGroupStrings.management.deleteControls.getCancel(), - title: ControlGroupStrings.management.deleteControls.getDeleteAllTitle(), - buttonColor: 'danger', - }).then((confirmed) => { - if (confirmed) - Object.keys(controlGroupContainer.getInput().panels).forEach((panelId) => - controlGroupContainer.removeEmbeddable(panelId) - ); - ref.close(); - }); - }; - - const flyoutInstance = openFlyout( - toMountPoint( - controlGroupContainer.updateInput(changes)} - controlCount={Object.keys(controlGroupContainer.getInput().panels ?? {}).length} - onDeleteAll={() => onDeleteAll(flyoutInstance)} - onClose={() => flyoutInstance.close()} - />, - { theme$ } - ), - { - outsideClickCloses: false, - onClose: () => { - flyoutInstance.close(); - setFlyoutRef(undefined); - }, - } - ); - setFlyoutRef(flyoutInstance); - }; - - const commonButtonProps = { - key: 'manageControls', - onClick: () => { - editControlGroup(); - closePopover(); - }, - icon: 'gear', - 'data-test-subj': 'controls-settings-button', - 'aria-label': ControlGroupStrings.management.getManageButtonTitle(), - }; - - return ( - - {ControlGroupStrings.management.getManageButtonTitle()} - - ); -}; diff --git a/src/plugins/controls/public/control_group/editor/open_add_data_control_flyout.tsx b/src/plugins/controls/public/control_group/editor/open_add_data_control_flyout.tsx index 8f426b2d310d8..0b44c09fbaabf 100644 --- a/src/plugins/controls/public/control_group/editor/open_add_data_control_flyout.tsx +++ b/src/plugins/controls/public/control_group/editor/open_add_data_control_flyout.tsx @@ -103,6 +103,8 @@ export function openAddDataControlFlyout(this: ControlGroupContainer) { onClose: () => { onCancel(); }, + // @ts-ignore - TODO: Remove this once https://github.com/elastic/eui/pull/6645 lands in Kibana + focusTrapProps: { scrollLock: true }, } ); setFlyoutRef(flyoutInstance); diff --git a/src/plugins/controls/public/control_group/editor/open_edit_control_group_flyout.tsx b/src/plugins/controls/public/control_group/editor/open_edit_control_group_flyout.tsx new file mode 100644 index 0000000000000..156ab377792dd --- /dev/null +++ b/src/plugins/controls/public/control_group/editor/open_edit_control_group_flyout.tsx @@ -0,0 +1,62 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import React from 'react'; +import { OverlayRef } from '@kbn/core-mount-utils-browser'; +import { toMountPoint } from '@kbn/kibana-react-plugin/public'; + +import { pluginServices } from '../../services'; +import { ControlGroupEditor } from './control_group_editor'; +import { ControlGroupStrings } from '../control_group_strings'; +import { ControlGroupContainer, setFlyoutRef } from '../embeddable/control_group_container'; + +export function openEditControlGroupFlyout(this: ControlGroupContainer) { + const { + overlays: { openFlyout, openConfirm }, + theme: { theme$ }, + } = pluginServices.getServices(); + const ReduxWrapper = this.getReduxEmbeddableTools().Wrapper; + + const onDeleteAll = (ref: OverlayRef) => { + openConfirm(ControlGroupStrings.management.deleteControls.getSubtitle(), { + confirmButtonText: ControlGroupStrings.management.deleteControls.getConfirm(), + cancelButtonText: ControlGroupStrings.management.deleteControls.getCancel(), + title: ControlGroupStrings.management.deleteControls.getDeleteAllTitle(), + buttonColor: 'danger', + }).then((confirmed) => { + if (confirmed) + Object.keys(this.getInput().panels).forEach((panelId) => this.removeEmbeddable(panelId)); + ref.close(); + }); + }; + + const flyoutInstance = openFlyout( + toMountPoint( + + this.updateInput(changes)} + controlCount={Object.keys(this.getInput().panels ?? {}).length} + onDeleteAll={() => onDeleteAll(flyoutInstance)} + onClose={() => flyoutInstance.close()} + /> + , + { theme$ } + ), + { + 'aria-label': ControlGroupStrings.manageControl.getFlyoutCreateTitle(), + outsideClickCloses: false, + onClose: () => { + this.closeAllFlyouts(); + }, + // @ts-ignore - TODO: Remove this once https://github.com/elastic/eui/pull/6645 lands in Kibana + focusTrapProps: { scrollLock: true }, + } + ); + setFlyoutRef(flyoutInstance); +} diff --git a/src/plugins/controls/public/control_group/embeddable/control_group_container.tsx b/src/plugins/controls/public/control_group/embeddable/control_group_container.tsx index 35b84656c7dbe..533f8ffa443fe 100644 --- a/src/plugins/controls/public/control_group/embeddable/control_group_container.tsx +++ b/src/plugins/controls/public/control_group/embeddable/control_group_container.tsx @@ -33,7 +33,6 @@ import { } from './control_group_chaining_system'; import { pluginServices } from '../../services'; import { openAddDataControlFlyout } from '../editor/open_add_data_control_flyout'; -import { EditControlGroup } from '../editor/edit_control_group'; import { ControlGroup } from '../component/control_group_component'; import { controlGroupReducers } from '../state/control_group_reducers'; import { ControlEmbeddable, ControlInput, ControlOutput } from '../../types'; @@ -49,6 +48,7 @@ import { getRangeSliderPanelState, getTimeSliderPanelState, } from '../control_group_input_builder'; +import { openEditControlGroupFlyout } from '../editor/open_edit_control_group_flyout'; let flyoutRef: OverlayRef | undefined; export const setFlyoutRef = (newRef: OverlayRef | undefined) => { @@ -124,15 +124,7 @@ export class ControlGroupContainer extends Container< public openAddDataControlFlyout = openAddDataControlFlyout; - public getEditControlGroupButton = (closePopover: () => void) => { - const ControlsServicesProvider = pluginServices.getContextProvider(); - - return ( - - - - ); - }; + public openEditControlGroupFlyout = openEditControlGroupFlyout; constructor( reduxEmbeddablePackage: ReduxEmbeddablePackage, diff --git a/src/plugins/controls/tsconfig.json b/src/plugins/controls/tsconfig.json index 5da6923aaaf5d..a386f5146e21b 100644 --- a/src/plugins/controls/tsconfig.json +++ b/src/plugins/controls/tsconfig.json @@ -35,6 +35,7 @@ "@kbn/ui-theme", "@kbn/safer-lodash-set", "@kbn/ui-actions-plugin", + "@kbn/core-mount-utils-browser", ], "exclude": [ "target/**/*", diff --git a/src/plugins/dashboard/public/dashboard_app/_dashboard_app_strings.ts b/src/plugins/dashboard/public/dashboard_app/_dashboard_app_strings.ts index 4144d9cd5b3af..26faaa87acad7 100644 --- a/src/plugins/dashboard/public/dashboard_app/_dashboard_app_strings.ts +++ b/src/plugins/dashboard/public/dashboard_app/_dashboard_app_strings.ts @@ -351,6 +351,11 @@ export const getAddControlButtonTitle = () => defaultMessage: 'Add control', }); +export const getEditControlGroupButtonTitle = () => + i18n.translate('dashboard.editingToolbar.editControlGroupButtonTitle', { + defaultMessage: 'Settings', + }); + export const getOnlyOneTimeSliderControlMsg = () => i18n.translate('dashboard.editingToolbar.onlyOneTimeSliderControlMsg', { defaultMessage: 'Control group already contains time slider control.', diff --git a/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/add_data_control_button.tsx b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/add_data_control_button.tsx index 72c68ce0b063b..6cef7e858b165 100644 --- a/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/add_data_control_button.tsx +++ b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/add_data_control_button.tsx @@ -16,10 +16,10 @@ interface Props { controlGroup: ControlGroupContainer; } -export const AddDataControlButton = ({ closePopover, controlGroup }: Props) => { +export const AddDataControlButton = ({ closePopover, controlGroup, ...rest }: Props) => { return ( { +export const AddTimeSliderControlButton = ({ closePopover, controlGroup, ...rest }: Props) => { const [hasTimeSliderControl, setHasTimeSliderControl] = useState(false); useEffect(() => { @@ -40,7 +40,7 @@ export const AddTimeSliderControlButton = ({ closePopover, controlGroup }: Props return ( { controlGroup.addTimeSliderControl(); diff --git a/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/controls_toolbar_button.tsx b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/controls_toolbar_button.tsx index 116f735fb6aac..1262ece75654a 100644 --- a/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/controls_toolbar_button.tsx +++ b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/controls_toolbar_button.tsx @@ -15,6 +15,7 @@ import type { ControlGroupContainer } from '@kbn/controls-plugin/public'; import { getControlButtonTitle } from '../../_dashboard_app_strings'; import { AddDataControlButton } from './add_data_control_button'; import { AddTimeSliderControlButton } from './add_time_slider_control_button'; +import { EditControlGroupButton } from './edit_control_group_button'; export function ControlsToolbarButton({ controlGroup }: { controlGroup: ControlGroupContainer }) { return ( @@ -27,9 +28,21 @@ export function ControlsToolbarButton({ controlGroup }: { controlGroup: ControlG {({ closePopover }: { closePopover: () => void }) => ( , - , - controlGroup.getEditControlGroupButton(closePopover), + , + , + , ]} /> )} diff --git a/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/edit_control_group_button.tsx b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/edit_control_group_button.tsx new file mode 100644 index 0000000000000..3563d87f5cf81 --- /dev/null +++ b/src/plugins/dashboard/public/dashboard_app/top_nav/controls_toolbar_button/edit_control_group_button.tsx @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import React from 'react'; +import { EuiContextMenuItem } from '@elastic/eui'; +import { ControlGroupContainer } from '@kbn/controls-plugin/public'; +import { getEditControlGroupButtonTitle } from '../../_dashboard_app_strings'; + +interface Props { + closePopover: () => void; + controlGroup: ControlGroupContainer; +} + +export const EditControlGroupButton = ({ closePopover, controlGroup, ...rest }: Props) => { + return ( + { + controlGroup.openEditControlGroupFlyout(); + closePopover(); + }} + > + {getEditControlGroupButtonTitle()} + + ); +}; diff --git a/src/plugins/discover/public/embeddable/saved_search_grid.tsx b/src/plugins/discover/public/embeddable/saved_search_grid.tsx index 716e403584cb7..a1e75e17dca5f 100644 --- a/src/plugins/discover/public/embeddable/saved_search_grid.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_grid.tsx @@ -21,7 +21,6 @@ export const DataGridMemoized = memo(DiscoverGrid); export function DiscoverGridEmbeddable(props: DiscoverGridEmbeddableProps) { const [expandedDoc, setExpandedDoc] = useState(undefined); - return ( - {props.totalHitCount !== 0 && ( + {Boolean(props.totalHitCount) && props.totalHitCount !== 0 && ( diff --git a/x-pack/plugins/translations/translations/fr-FR.json b/x-pack/plugins/translations/translations/fr-FR.json index d5551920447e5..6fdcfd331f1a7 100644 --- a/x-pack/plugins/translations/translations/fr-FR.json +++ b/x-pack/plugins/translations/translations/fr-FR.json @@ -417,7 +417,6 @@ "controls.controlGroup.manageControl.titleInputTitle": "Étiquette", "controls.controlGroup.manageControl.widthInputTitle": "Largeur minimale", "controls.controlGroup.management.addControl": "Ajouter un contrôle", - "controls.controlGroup.management.buttonTitle": "Paramètres", "controls.controlGroup.management.delete": "Supprimer le contrôle", "controls.controlGroup.management.delete.cancel": "Annuler", "controls.controlGroup.management.delete.confirm": "Supprimer", diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index e24613581c591..069da5ff80bdc 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -417,7 +417,6 @@ "controls.controlGroup.manageControl.titleInputTitle": "ラベル", "controls.controlGroup.manageControl.widthInputTitle": "最小幅", "controls.controlGroup.management.addControl": "コントロールを追加", - "controls.controlGroup.management.buttonTitle": "設定", "controls.controlGroup.management.delete": "コントロールを削除", "controls.controlGroup.management.delete.cancel": "キャンセル", "controls.controlGroup.management.delete.confirm": "削除", diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 01f38e9091d8e..aeae49eb9a47d 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -417,7 +417,6 @@ "controls.controlGroup.manageControl.titleInputTitle": "标签", "controls.controlGroup.manageControl.widthInputTitle": "最小宽度", "controls.controlGroup.management.addControl": "添加控件", - "controls.controlGroup.management.buttonTitle": "设置", "controls.controlGroup.management.delete": "删除控件", "controls.controlGroup.management.delete.cancel": "取消", "controls.controlGroup.management.delete.confirm": "删除", diff --git a/yarn.lock b/yarn.lock index 0306138662601..ecb6fc6004202 100644 --- a/yarn.lock +++ b/yarn.lock @@ -24184,9 +24184,9 @@ react-refresh@^0.11.0: integrity sha512-F27qZr8uUqwhWZboondsPx8tnC3Ct3SxZA3V5WyEvujRyyNv0VYPhoBg1gZ8/MV5tubQp76Trw8lTv9hzRBa+A== react-remove-scroll-bar@^2.3.3: - version "2.3.3" - resolved "https://registry.yarnpkg.com/react-remove-scroll-bar/-/react-remove-scroll-bar-2.3.3.tgz#e291f71b1bb30f5f67f023765b7435f4b2b2cd94" - integrity sha512-i9GMNWwpz8XpUpQ6QlevUtFjHGqnPG4Hxs+wlIJntu/xcsZVEpJcIV71K3ZkqNy2q3GfgvkD7y6t/Sv8ofYSbw== + version "2.3.4" + resolved "https://registry.yarnpkg.com/react-remove-scroll-bar/-/react-remove-scroll-bar-2.3.4.tgz#53e272d7a5cb8242990c7f144c44d8bd8ab5afd9" + integrity sha512-63C4YQBUt0m6ALadE9XV56hV8BgJWDmmTPY758iIJjfQKt2nYwoUrPk0LXRXcB/yIj82T1/Ixfdpdk68LwIB0A== dependencies: react-style-singleton "^2.2.1" tslib "^2.0.0"