Skip to content

Commit

Permalink
[Dashboard] Fix React errors (#153320)
Browse files Browse the repository at this point in the history
Closes #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 👍

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 <[email protected]>
  • Loading branch information
Heenawter and kibanamachine authored Mar 22, 2023
1 parent b34a4ec commit 8fa826d
Show file tree
Hide file tree
Showing 16 changed files with 130 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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(
<ReduxWrapper>
<ControlGroupEditor
initialInput={this.getInput()}
updateInput={(changes) => this.updateInput(changes)}
controlCount={Object.keys(this.getInput().panels ?? {}).length}
onDeleteAll={() => onDeleteAll(flyoutInstance)}
onClose={() => flyoutInstance.close()}
/>
</ReduxWrapper>,
{ 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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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) => {
Expand Down Expand Up @@ -124,15 +124,7 @@ export class ControlGroupContainer extends Container<

public openAddDataControlFlyout = openAddDataControlFlyout;

public getEditControlGroupButton = (closePopover: () => void) => {
const ControlsServicesProvider = pluginServices.getContextProvider();

return (
<ControlsServicesProvider>
<EditControlGroup controlGroupContainer={this} closePopover={closePopover} />
</ControlsServicesProvider>
);
};
public openEditControlGroupFlyout = openEditControlGroupFlyout;

constructor(
reduxEmbeddablePackage: ReduxEmbeddablePackage,
Expand Down
1 change: 1 addition & 0 deletions src/plugins/controls/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@kbn/ui-theme",
"@kbn/safer-lodash-set",
"@kbn/ui-actions-plugin",
"@kbn/core-mount-utils-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ interface Props {
controlGroup: ControlGroupContainer;
}

export const AddDataControlButton = ({ closePopover, controlGroup }: Props) => {
export const AddDataControlButton = ({ closePopover, controlGroup, ...rest }: Props) => {
return (
<EuiContextMenuItem
key="addControl"
{...rest}
icon="plusInCircle"
data-test-subj="controls-create-button"
aria-label={getAddControlButtonTitle()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ interface Props {
controlGroup: ControlGroupContainer;
}

export const AddTimeSliderControlButton = ({ closePopover, controlGroup }: Props) => {
export const AddTimeSliderControlButton = ({ closePopover, controlGroup, ...rest }: Props) => {
const [hasTimeSliderControl, setHasTimeSliderControl] = useState(false);

useEffect(() => {
Expand All @@ -40,7 +40,7 @@ export const AddTimeSliderControlButton = ({ closePopover, controlGroup }: Props

return (
<EuiContextMenuItem
key="addTimeSliderControl"
{...rest}
icon="plusInCircle"
onClick={() => {
controlGroup.addTimeSliderControl();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -27,9 +28,21 @@ export function ControlsToolbarButton({ controlGroup }: { controlGroup: ControlG
{({ closePopover }: { closePopover: () => void }) => (
<EuiContextMenuPanel
items={[
<AddDataControlButton controlGroup={controlGroup} closePopover={closePopover} />,
<AddTimeSliderControlButton controlGroup={controlGroup} closePopover={closePopover} />,
controlGroup.getEditControlGroupButton(closePopover),
<AddDataControlButton
key="addControl"
controlGroup={controlGroup}
closePopover={closePopover}
/>,
<AddTimeSliderControlButton
key="addTimeSliderControl"
controlGroup={controlGroup}
closePopover={closePopover}
/>,
<EditControlGroupButton
key="manageControls"
controlGroup={controlGroup}
closePopover={closePopover}
/>,
]}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 (
<EuiContextMenuItem
{...rest}
icon="gear"
data-test-subj="controls-settings-button"
aria-label={getEditControlGroupButtonTitle()}
onClick={() => {
controlGroup.openEditControlGroupFlyout();
closePopover();
}}
>
{getEditControlGroupButtonTitle()}
</EuiContextMenuItem>
);
};
3 changes: 1 addition & 2 deletions src/plugins/discover/public/embeddable/saved_search_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export const DataGridMemoized = memo(DiscoverGrid);

export function DiscoverGridEmbeddable(props: DiscoverGridEmbeddableProps) {
const [expandedDoc, setExpandedDoc] = useState<DataTableRecord | undefined>(undefined);

return (
<EuiFlexGroup
style={{ width: '100%' }}
Expand All @@ -30,7 +29,7 @@ export function DiscoverGridEmbeddable(props: DiscoverGridEmbeddableProps) {
responsive={false}
data-test-subj="embeddedSavedSearchDocTable"
>
{props.totalHitCount !== 0 && (
{Boolean(props.totalHitCount) && props.totalHitCount !== 0 && (
<EuiFlexItem grow={false} style={{ alignSelf: 'flex-end' }}>
<TotalDocuments totalHitCount={props.totalHitCount} />
</EuiFlexItem>
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/fr-FR.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": "削除",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": "删除",
Expand Down
6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 8fa826d

Please sign in to comment.