Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved Visualize button in field popover #103099

Merged
merged 5 commits into from
Jun 24, 2021
+288 −1,038
Merged

Improved Visualize button in field popover #103099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
279b72d
Improve field popover
Jun 23, 2021
7223bc5
Slightly improve type safteyness
Jun 23, 2021
52766d6
Add unit tests for visualize trigger utils
Jun 23, 2021
fd9ed8e
Remove unused div
Jun 23, 2021
19366b0
Merge branch 'master' into discover/visualize-button-improvements
kibanamachine Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
705 changes: 0 additions & 705 deletions ...on/apps/main/components/sidebar/__snapshots__/discover_field_details_footer.test.tsx.snap
View file
Open in desktop

This file was deleted.

37 changes: 21 additions & 16 deletions src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import './discover_field.scss';

import React, { useState, useCallback, memo } from 'react';
import React, { useState, useCallback, memo, useMemo } from 'react';
import {
EuiPopover,
EuiPopoverTitle,
Expand All @@ -18,6 +18,7 @@ import {
EuiIcon,
EuiFlexGroup,
EuiFlexItem,
EuiSpacer,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { UiCounterMetricType } from '@kbn/analytics';
Expand All @@ -27,7 +28,7 @@ import { FieldIcon, FieldButton } from '../../../../../../../kibana_react/public
import { FieldDetails } from './types';
import { IndexPatternField, IndexPattern } from '../../../../../../../data/public';
import { getFieldTypeName } from './lib/get_field_type_name';
import { DiscoverFieldDetailsFooter } from './discover_field_details_footer';
import { DiscoverFieldVisualize } from './discover_field_visualize';

function wrapOnDot(str?: string) {
// u200B is a non-width white-space character, which allows
Expand Down Expand Up @@ -172,6 +173,7 @@ const MultiFields: React.FC<MultiFieldsProps> = memo(
})}
</h5>
</EuiTitle>
<EuiSpacer size="xs" />
{multiFields.map((entry) => (
<FieldButton
size="s"
Expand Down Expand Up @@ -282,6 +284,8 @@ function DiscoverFieldComponent({
setOpen(!infoIsOpen);
}, [infoIsOpen]);

const rawMultiFields = useMemo(() => multiFields?.map((f) => f.field), [multiFields]);

if (field.type === '_source') {
return (
<FieldButton
Expand Down Expand Up @@ -399,23 +403,24 @@ function DiscoverFieldComponent({
field={field}
details={details}
onAddFilter={onAddFilter}
trackUiMetric={trackUiMetric}
/>
{multiFields && (
<MultiFields
multiFields={multiFields}
alwaysShowActionButton={alwaysShowActionButton}
toggleDisplay={toggleDisplay}
/>
)}
{!details.error && (
<DiscoverFieldDetailsFooter
indexPattern={indexPattern}
field={field}
details={details}
onAddFilter={onAddFilter}
/>
<>
<EuiSpacer size="m" />
<MultiFields
multiFields={multiFields}
alwaysShowActionButton={alwaysShowActionButton}
toggleDisplay={toggleDisplay}
/>
</>
)}
<DiscoverFieldVisualize
field={field}
indexPattern={indexPattern}
multiFields={rawMultiFields}
trackUiMetric={trackUiMetric}
details={details}
/>
</>
)}
</EuiPopover>
Expand Down
10 changes: 0 additions & 10 deletions ...gins/discover/public/application/apps/main/components/sidebar/discover_field_details.scss
View file
Open in desktop

This file was deleted.

41 changes: 7 additions & 34 deletions .../discover/public/application/apps/main/components/sidebar/discover_field_details.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,19 @@ const indexPattern = getStubIndexPattern(
);

describe('discover sidebar field details', function () {
const onAddFilter = jest.fn();
const defaultProps = {
indexPattern,
details: { buckets: [], error: '', exists: 1, total: 2, columns: [] },
onAddFilter: jest.fn(),
onAddFilter,
};

function mountComponent(field: IndexPatternField) {
const compProps = { ...defaultProps, field };
return mountWithIntl(<DiscoverFieldDetails {...compProps} />);
}

it('should enable the visualize link for a number field', function () {
Copy link
Contributor Author

@timroes timroes Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ I've removed all those tests, since they were anyway not working. The only test that checked if the button was visible, did not do that correctly (since the result of findTestSubject will always be truthy and a .exists() check is required). If that test would have been tested properly it would have always failed, due to the async nature of the component. I instead decided to replace this by unit tests for the utility class to load that visualization information.

it('click on addFilter calls the function', function () {
const visualizableField = new IndexPatternField({
name: 'bytes',
type: 'number',
Expand All @@ -47,37 +48,9 @@ describe('discover sidebar field details', function () {
aggregatable: true,
readFromDocValues: true,
});
const comp = mountComponent(visualizableField);
expect(findTestSubject(comp, 'fieldVisualize-bytes')).toBeTruthy();
});

it('should disable the visualize link for an _id field', function () {
const conflictField = new IndexPatternField({
name: '_id',
type: 'string',
esTypes: ['_id'],
count: 0,
scripted: false,
searchable: true,
aggregatable: true,
readFromDocValues: true,
});
const comp = mountComponent(conflictField);
expect(findTestSubject(comp, 'fieldVisualize-_id')).toEqual({});
});

it('should disable the visualize link for an unknown field', function () {
const unknownField = new IndexPatternField({
name: 'test',
type: 'unknown',
esTypes: ['double'],
count: 0,
scripted: false,
searchable: true,
aggregatable: true,
readFromDocValues: true,
});
const comp = mountComponent(unknownField);
expect(findTestSubject(comp, 'fieldVisualize-test')).toEqual({});
const component = mountComponent(visualizableField);
const onAddButton = findTestSubject(component, 'onAddFilterButton');
onAddButton.simulate('click');
expect(onAddFilter).toHaveBeenCalledWith('_exists_', visualizableField.name, '+');
Comment on lines +51 to +54
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This test got moved from the now deleted "Footer" component.

});
});
100 changes: 31 additions & 69 deletions ...ugins/discover/public/application/apps/main/components/sidebar/discover_field_details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,74 +6,31 @@
* Side Public License, v 1.
*/

import React, { useState, useEffect } from 'react';
import { EuiIconTip, EuiText, EuiButton, EuiSpacer } from '@elastic/eui';
import React from 'react';
import { EuiText, EuiSpacer, EuiLink } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { METRIC_TYPE, UiCounterMetricType } from '@kbn/analytics';
import { DiscoverFieldBucket } from './discover_field_bucket';
import { getWarnings } from './lib/get_warnings';
import {
triggerVisualizeActions,
isFieldVisualizable,
getVisualizeHref,
} from './lib/visualize_trigger_utils';
import { Bucket, FieldDetails } from './types';
import { IndexPatternField, IndexPattern } from '../../../../../../../data/public';
import './discover_field_details.scss';

interface DiscoverFieldDetailsProps {
field: IndexPatternField;
indexPattern: IndexPattern;
details: FieldDetails;
onAddFilter: (field: IndexPatternField | string, value: string, type: '+' | '-') => void;
trackUiMetric?: (metricType: UiCounterMetricType, eventName: string | string[]) => void;
}

export function DiscoverFieldDetails({
field,
indexPattern,
details,
onAddFilter,
trackUiMetric,
}: DiscoverFieldDetailsProps) {
const warnings = getWarnings(field);
const [showVisualizeLink, setShowVisualizeLink] = useState<boolean>(false);
const [visualizeLink, setVisualizeLink] = useState<string>('');

useEffect(() => {
isFieldVisualizable(field, indexPattern.id, details.columns).then(
(flag) => {
setShowVisualizeLink(flag);
// get href only if Visualize button is enabled
getVisualizeHref(field, indexPattern.id, details.columns).then(
(uri) => {
if (uri) setVisualizeLink(uri);
},
() => {
setVisualizeLink('');
}
);
},
() => {
setShowVisualizeLink(false);
}
);
}, [field, indexPattern.id, details.columns]);

const handleVisualizeLinkClick = (event: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
// regular link click. let the uiActions code handle the navigation and show popup if needed
event.preventDefault();
if (trackUiMetric) {
trackUiMetric(METRIC_TYPE.CLICK, 'visualize_link_click');
}
triggerVisualizeActions(field, indexPattern.id, details.columns);
};

return (
<>
<div className="dscFieldDetails">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This CSS class just added a couple of spacing (I replaced that by some well placed <EuiSpacer />) and a redundant text color. This allowed us to get rid of the whole SCSS for that component.

{details.error && <EuiText size="xs">{details.error}</EuiText>}
{!details.error && (
{details.error && <EuiText size="xs">{details.error}</EuiText>}
{!details.error && (
<>
<div style={{ marginTop: '4px' }}>
{details.buckets.map((bucket: Bucket, idx: number) => (
<DiscoverFieldBucket
Expand All @@ -84,30 +41,35 @@ export function DiscoverFieldDetails({
/>
))}
</div>
)}

{showVisualizeLink && (
<>
<EuiSpacer size="xs" />
{/* eslint-disable-next-line @elastic/eui/href-or-on-click */}
<EuiButton
onClick={(e) => handleVisualizeLinkClick(e)}
href={visualizeLink}
size="s"
className="dscFieldDetails__visualizeBtn"
data-test-subj={`fieldVisualize-${field.name}`}
>
<EuiSpacer size="xs" />
<EuiText size="xs">
{!indexPattern.metaFields.includes(field.name) && !field.scripted ? (
<EuiLink
onClick={() => onAddFilter('_exists_', field.name, '+')}
data-test-subj="onAddFilterButton"
>
<FormattedMessage
id="discover.fieldChooser.detailViews.existsInRecordsText"
defaultMessage="Exists in {value} / {totalValue} records"
values={{
value: details.exists,
totalValue: details.total,
}}
/>
</EuiLink>
) : (
<FormattedMessage
id="discover.fieldChooser.detailViews.visualizeLinkText"
defaultMessage="Visualize"
id="discover.fieldChooser.detailViews.valueOfRecordsText"
defaultMessage="{value} / {totalValue} records"
values={{
value: details.exists,
totalValue: details.total,
}}
/>
</EuiButton>
{warnings.length > 0 && (
<EuiIconTip type="alert" color="warning" content={warnings.join(' ')} />
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This is the mentioned scripted field performance warning that's removed (it was never possible to show any other warning than that one).

</>
)}
</div>
</EuiText>
</>
)}
</>
);
}
71 changes: 0 additions & 71 deletions ...er/public/application/apps/main/components/sidebar/discover_field_details_footer.test.tsx
View file
Open in desktop

This file was deleted.

Loading