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

Jump to bottom

Add tech preview label for search applications #155649

Merged
merged 7 commits into from
Apr 26, 2023
Merged

Add tech preview label for search applications #155649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4e6c110
Add tech preview label for search applications
ioanatia Apr 24, 2023
8719ae5
Copy changes
ioanatia Apr 25, 2023
7814259
Add beaker icon in popover title
ioanatia Apr 25, 2023
589f29a
Use FormattedMessage
ioanatia Apr 25, 2023
12e875f
Merge branch 'main' into search_apps_tech_preview
ioanatia Apr 25, 2023
5e9d4bc
Address review comments
ioanatia Apr 25, 2023
9e3b5a8
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Apr 25, 2023
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
92 changes: 52 additions & 40 deletions ...public/applications/enterprise_search_content/components/engines/create_engine_flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export const CreateEngineFlyout = ({ onClose }: CreateEngineFlyoutProps) => {
<p>
<FormattedMessage
id="xpack.enterpriseSearch.content.engines.createEngine.headerSubTitle"
defaultMessage="A Search Application allows your users to query data in your indices. Explore our {enginesDocsLink} to learn more!"
defaultMessage="Explore our {enginesDocsLink} to learn more!"
values={{
enginesDocsLink: (
<EuiLink
Expand Down Expand Up @@ -127,44 +127,56 @@ export const CreateEngineFlyout = ({ onClose }: CreateEngineFlyoutProps) => {
)}
</EuiFlyoutHeader>
<EuiFlyoutBody>
<EuiSteps
steps={[
{
children: (
<IndicesSelectComboBox
fullWidth
isDisabled={formDisabled}
onChange={onIndicesChange}
selectedOptions={selectedIndices.map((index: string) => indexToOption(index))}
/>
),
status: indicesStatus,
title: i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.selectIndices.title',
{ defaultMessage: 'Select indices' }
),
},
{
children: (
<EuiFieldText
fullWidth
disabled={formDisabled}
placeholder={i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.nameEngine.placeholder',
{ defaultMessage: 'Search Application name' }
)}
value={engineName}
onChange={(e) => setEngineName(e.target.value)}
/>
),
status: engineNameStatus,
title: i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.nameEngine.title',
{ defaultMessage: 'Name your Search Application' }
),
},
]}
/>
<EuiFlexGroup direction="column">
<EuiFlexItem grow>
<EuiCallOut title="Technical Preview feature" color="warning" iconType="beaker">
<FormattedMessage
id="xpack.enterpriseSearch.content.engines.createEngine.technicalPreviewCallOut.title"
defaultMessage="This functionality is in technical preview and may be changed or removed completely in a future release. Elastic will take a best effort approach to fix any issues, but features in technical preview are not subject to the support SLA of official GA features."
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we quickly change this to say:

This functionality may be changed or removed completely in a future release.

To make it less wordy

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized this was in the wrong place, need this for the popover, not the callout. Callout is fine as is.

/>
</EuiCallOut>
</EuiFlexItem>
<EuiFlexItem grow>
<EuiSteps
steps={[
{
children: (
<IndicesSelectComboBox
fullWidth
isDisabled={formDisabled}
onChange={onIndicesChange}
selectedOptions={selectedIndices.map((index: string) => indexToOption(index))}
/>
),
status: indicesStatus,
title: i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.selectIndices.title',
{ defaultMessage: 'Select indices' }
),
},
{
children: (
<EuiFieldText
fullWidth
disabled={formDisabled}
placeholder={i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.nameEngine.placeholder',
{ defaultMessage: 'Search application name' }
)}
value={engineName}
onChange={(e) => setEngineName(e.target.value)}
/>
),
status: engineNameStatus,
title: i18n.translate(
'xpack.enterpriseSearch.content.engines.createEngine.nameEngine.title',
{ defaultMessage: 'Name your search application' }
),
},
]}
/>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlyoutBody>
<EuiFlyoutFooter>
<EuiFlexGroup>
Expand All @@ -189,7 +201,7 @@ export const CreateEngineFlyout = ({ onClose }: CreateEngineFlyoutProps) => {
}}
>
{i18n.translate('xpack.enterpriseSearch.content.engines.createEngine.submit', {
defaultMessage: 'Create this Search Application',
defaultMessage: 'Create',
})}
</EuiButton>
</EuiFlexItem>
Expand Down
65 changes: 8 additions & 57 deletions ...ch/public/applications/enterprise_search_content/components/engines/engines_list.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,34 +120,6 @@ describe('EnginesList', () => {

describe('CreateEngineButton', () => {
describe('disabled={true}', () => {
it('renders a disabled button that shows a popover when focused', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I repurposed the popover for the platinum license acknowledgement based on the design.
The initial platinum license popover was only visible when not on an platinum+/trial license (and hidden on trial/platinum+)
The Create Search Application would also be disabled when not on platinum+/trial.
Because the popover is now visible regardless of the license level and whether the create button is disabled, I removed part of the tests.

const wrapper = mount(<CreateEngineButton disabled />);

const button = wrapper.find(
'button[data-test-subj="enterprise-search-content-engines-creation-button"]'
);

expect(button).toHaveLength(1);
expect(button.prop('disabled')).toBeTruthy();

let popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(0);

const hoverTarget = wrapper.find('div[data-test-subj="create-engine-button-hover-target"]');

expect(hoverTarget).toHaveLength(1);

hoverTarget.simulate('focus');

wrapper.update();

popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(1);
expect(popover.text()).toMatch('Search Applications require a Platinum license');
});

it('renders a disabled button that shows a popover when hovered', () => {
const wrapper = mount(<CreateEngineButton disabled />);

Expand All @@ -173,37 +145,13 @@ describe('CreateEngineButton', () => {
popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(1);
expect(popover.text()).toMatch('Search Applications require a Platinum license');
expect(popover.text()).toMatch(
'This functionality is in technical preview and may be changed or removed completely in a future release.'
);
});
});
describe('disabled={false}', () => {
it('renders a button and does not show a popover when focused', () => {
const wrapper = mount(<CreateEngineButton disabled={false} />);

const button = wrapper.find(
'button[data-test-subj="enterprise-search-content-engines-creation-button"]'
);

expect(button).toHaveLength(1);
expect(button.prop('disabled')).toBeFalsy();

let popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(0);

const hoverTarget = wrapper.find('div[data-test-subj="create-engine-button-hover-target"]');

expect(hoverTarget).toHaveLength(1);

hoverTarget.simulate('focus');

wrapper.update();

popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(0);
});
it('renders a button and does not show a popover when hovered', () => {
it('renders a button and shows a popover when hovered', () => {
const wrapper = mount(<CreateEngineButton disabled={false} />);

const button = wrapper.find(
Expand All @@ -227,7 +175,10 @@ describe('CreateEngineButton', () => {

popover = wrapper.find('div[data-test-subj="create-engine-button-popover-content"]');

expect(popover).toHaveLength(0);
expect(popover).toHaveLength(1);
expect(popover.text()).toMatch(
'This functionality is in technical preview and may be changed or removed completely in a future release.'
);
});
});
});
30 changes: 17 additions & 13 deletions ..._search/public/applications/enterprise_search_content/components/engines/engines_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
EuiFieldSearch,
EuiFlexGroup,
EuiFlexItem,
EuiIcon,
EuiLink,
EuiPopover,
EuiPopoverTitle,
Expand All @@ -30,7 +31,6 @@ import { docLinks } from '../../../shared/doc_links';

import { KibanaLogic } from '../../../shared/kibana';
import { LicensingLogic } from '../../../shared/licensing';
import { EXPLORE_PLATINUM_FEATURES_LINK } from '../../../workplace_search/constants';
import { ENGINES_PATH, ENGINE_CREATION_PATH } from '../../routes';
import { EnterpriseSearchEnginesPageTemplate } from '../layout/engines_page_template';

Expand All @@ -52,13 +52,13 @@ export const CreateEngineButton: React.FC<CreateEngineButtonProps> = ({ disabled

return (
<EuiPopover
isOpen={disabled && showPopover}
isOpen={showPopover}
closePopover={() => setShowPopover(false)}
button={
<div
data-test-subj="create-engine-button-hover-target"
onMouseEnter={() => setShowPopover(true)}
onFocus={() => setShowPopover(true)}
onMouseLeave={() => setShowPopover(false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

we may still want a focus event for users navigating with a keyboard

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should definitely have that. I am also concerned about other potential accessibility issues here. Does the popover announce its presence when opened? Does tabIndex 0 make sense in the context of this page? Does onFocus get triggered when someone tabs to the button, or only when it tabs to the div?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get these fixed in a follow up PR. thanks for the feedback

tabIndex={0}
>
<EuiButton
Expand All @@ -72,30 +72,34 @@ export const CreateEngineButton: React.FC<CreateEngineButtonProps> = ({ disabled
{i18n.translate(
'xpack.enterpriseSearch.content.searchApplications.createEngineButtonLabel',
{
defaultMessage: 'Create Search Application',
defaultMessage: 'Create',
}
)}
</EuiButton>
</div>
}
>
<EuiPopoverTitle>
<FormattedMessage
id="xpack.enterpriseSearch.content.searchApplications.createEngineDisabledPopover.title"
defaultMessage="Platinum only feature"
/>
<EuiFlexGroup justifyContent="center" gutterSize="s">
<EuiFlexItem grow={false}>
<EuiIcon type="beaker" />
</EuiFlexItem>
<EuiFlexItem grow={false}>
<FormattedMessage
id="xpack.enterpriseSearch.content.searchApplications.createEngineTechnicalPreviewPopover.title"
defaultMessage="Technical Preview"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why (in your screenshot) there is so much space between the icon and the title text but wanted to flag

CleanShot 2023-04-25 at 09 21 05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like the space is controlled by gutterSize TIL
should look a bit more decent now:

Screenshot 2023-04-25 at 17 23 32

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me Ioana!

</EuiFlexItem>
</EuiFlexGroup>
</EuiPopoverTitle>
<div style={{ width: '300px' }} data-test-subj="create-engine-button-popover-content">
<EuiFlexGroup direction="column" gutterSize="m">
<EuiText size="s">
<FormattedMessage
id="xpack.enterpriseSearch.content.searchApplications.createEngineDisabledPopover.body"
defaultMessage="Search Applications require a Platinum license or higher and are not available to Standard license self-managed deployments."
id="xpack.enterpriseSearch.content.searchApplications.createEngineTechnicalPreviewPopover.body"
defaultMessage="This functionality is in technical preview and may be changed or removed completely in a future release. Elastic will take a best effort approach to fix any issues, but features in technical preview are not subject to the support SLA of official GA features."
/>
</EuiText>
<EuiLink target="_blank" href={docLinks.licenseManagement}>
{EXPLORE_PLATINUM_FEATURES_LINK}
</EuiLink>
</EuiFlexGroup>
</div>
</EuiPopover>
Expand Down