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

[Workplace Search] Convert Settings pages to new page template #102445

Merged
merged 6 commits into from
Jun 17, 2021
Merged

[Workplace Search] Convert Settings pages to new page template #102445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9240f4b
Convert Settings > Customize to new page template
cee-chen Jun 17, 2021
cc36617
Convert Settings > oAuth application to new page template
cee-chen Jun 17, 2021
0fc182b
Convert Settings > Connectors to new page template
cee-chen Jun 17, 2021
cfe6d65
Convert source config view to new page template
cee-chen Jun 17, 2021
2fbe05b
Convert Settings subnav to EuiSideNav format
cee-chen Jun 17, 2021
d99d518
Update routers
cee-chen Jun 17, 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
3 changes: 3 additions & 0 deletions ...ins/enterprise_search/public/applications/workplace_search/components/layout/nav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ jest.mock('../../../shared/layout', () => ({
...jest.requireActual('../../../shared/layout'),
generateNavLink: jest.fn(({ to }) => ({ href: to })),
}));
jest.mock('../../views/settings/components/settings_sub_nav', () => ({
useSettingsSubNav: () => [],
}));

import React from 'react';

Expand Down
3 changes: 2 additions & 1 deletion .../plugins/enterprise_search/public/applications/workplace_search/components/layout/nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
GROUPS_PATH,
ORG_SETTINGS_PATH,
} from '../../routes';
import { useSettingsSubNav } from '../../views/settings/components/settings_sub_nav';
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: I associate "use" with React hooks. What do you think about renaming this to getSettingsSubNav? Totally optional, this definitely works. If you agree feel free to do it in a different PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

++ I thought the same thing when I saw it.

Copy link
Contributor Author

@cee-chen cee-chen Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is a React hook actually! And (unfortunately) it does has to be one/have the use prefix in order for the function itself to use hooks, e.g. useRouteMatch, useValues, etc. - we'll get errors and crashes otherwise. I did try a simple getXNav but no dice, couldn't get it to work 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

homerhide

Copy link
Contributor Author

@cee-chen cee-chen Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also since we're discussing this here, I wanted to mention some unfortunately mildly annoying shenanigans with the useXSubNav hook pattern:

  1. Hooks can't ever be called conditionally. I tried to do things like this (for performance):
items: useRouteMatch(SOURCES_PATH) ? useSourceSubNav() : []

But I got React errors because you're supposed to always call hooks in a set order. So I fell back to the hook itself returning early/undefined instead.

  1. You can't use a hook within a hook/callback - I saw that the Observability folks were using useMemo to memoize their side nav items/reduce rerenders and wanted to copy that, but unfortunately we can't nest a useXSubNav within a useMemo. It's probably not a huge deal and our app is still super responsive, but it definitely feels like it would be nice to get around this.

The primary issue is, without it being a useSubNav fn, we can't use the Kea useValues hooks. And unfortunately we do need the actual useValues hook, we can't simply do SourceLogic.values.contentSource.id - it crashes/errors on pages where SourceLogic isn't mounted.

There are a few ways I can think of to work around these limitations, but they're potentially complicated and require adding new infrastructure (e.g.: hook checks within the main nav itself but not the sub navs? useParams? an always-mounted logic file just for the nav which stores source/group IDs so we can avoid useValues?) and I tried to make this conversion as simple as possible for now.

I'm definitely interested in coming back to see if we can avoid hooks entirely for the sub navs, or someone else is totally welcome to look, but also we may be better off punting until 7.15 or until it becomes a major pain point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that punting is fine for now.


export const useWorkplaceSearchNav = () => {
const navItems: Array<EuiSideNavItemType<unknown>> = [
Expand Down Expand Up @@ -53,7 +54,7 @@ export const useWorkplaceSearchNav = () => {
id: 'settings',
name: NAV.SETTINGS,
...generateNavLink({ to: ORG_SETTINGS_PATH }),
items: [], // TODO: Settings subnav
items: useSettingsSubNav(),
},
];

Expand Down
9 changes: 1 addition & 8 deletions x-pack/plugins/enterprise_search/public/applications/workplace_search/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import { Overview } from './views/overview';
import { RoleMappings } from './views/role_mappings';
import { Security } from './views/security';
import { SettingsRouter } from './views/settings';
import { SettingsSubNav } from './views/settings/components/settings_sub_nav';
import { SetupGuide } from './views/setup_guide';

export const WorkplaceSearch: React.FC<InitialAppData> = (props) => {
Expand Down Expand Up @@ -149,13 +148,7 @@ export const WorkplaceSearchConfigured: React.FC<InitialAppData> = (props) => {
</Layout>
</Route>
<Route path={ORG_SETTINGS_PATH}>
<Layout
navigation={<WorkplaceSearchNav settingsSubNav={<SettingsSubNav />} />}
restrictWidth
readOnlyMode={readOnlyMode}
>
<SettingsRouter />
</Layout>
<SettingsRouter />
</Route>
<Route>
<Layout navigation={<WorkplaceSearchNav />} restrictWidth readOnlyMode={readOnlyMode}>
Expand Down
11 changes: 0 additions & 11 deletions ...search/public/applications/workplace_search/views/settings/components/connectors.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import React from 'react';

import { shallow } from 'enzyme';

import { Loading } from '../../../../shared/loading';
import { LicenseCallout } from '../../../components/shared/license_callout';

import { Connectors } from './connectors';
Expand All @@ -33,16 +32,6 @@ describe('Connectors', () => {
expect(wrapper.find('[data-test-subj="ConnectorRow"]')).toHaveLength(configuredSources.length);
});

it('returns loading when loading', () => {
setMockValues({
connectors: configuredSources,
dataLoading: true,
});
const wrapper = shallow(<Connectors />);

expect(wrapper.find(Loading)).toHaveLength(1);
});

it('renders LicenseCallout for restricted items', () => {
const wrapper = shallow(<Connectors />);

Expand Down
21 changes: 11 additions & 10 deletions ...rise_search/public/applications/workplace_search/views/settings/components/connectors.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ import {
EuiSpacer,
} from '@elastic/eui';

import { Loading } from '../../../../shared/loading';
import { EuiButtonEmptyTo } from '../../../../shared/react_router_helpers';
import { WorkplaceSearchPageTemplate } from '../../../components/layout';
import { LicenseCallout } from '../../../components/shared/license_callout';
import { SourceIcon } from '../../../components/shared/source_icon';
import { ViewContentHeader } from '../../../components/shared/view_content_header';
import {
NAV,
CONFIGURE_BUTTON,
CONNECTORS_HEADER_TITLE,
CONNECTORS_HEADER_DESCRIPTION,
Expand All @@ -46,8 +46,6 @@ export const Connectors: React.FC = () => {
initializeConnectors();
}, []);

if (dataLoading) return <Loading />;

const availableConnectors = reject(
connectors,
({ serviceType }) => serviceType === CUSTOM_SERVICE_TYPE
Expand Down Expand Up @@ -125,12 +123,15 @@ export const Connectors: React.FC = () => {
);

return (
<>
<ViewContentHeader
title={CONNECTORS_HEADER_TITLE}
description={CONNECTORS_HEADER_DESCRIPTION}
/>
<WorkplaceSearchPageTemplate
pageChrome={[NAV.SETTINGS, NAV.SETTINGS_SOURCE_PRIORITIZATION]}
pageHeader={{
pageTitle: CONNECTORS_HEADER_TITLE,
description: CONNECTORS_HEADER_DESCRIPTION,
}}
isLoading={dataLoading}
>
{connectorsList}
</>
</WorkplaceSearchPageTemplate>
);
};
61 changes: 33 additions & 28 deletions ...prise_search/public/applications/workplace_search/views/settings/components/customize.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { useActions, useValues } from 'kea';

import { EuiButton, EuiFieldText, EuiFlexGroup, EuiFlexItem, EuiFormRow } from '@elastic/eui';

import { WorkplaceSearchPageTemplate } from '../../../components/layout';
import { ContentSection } from '../../../components/shared/content_section';
import { ViewContentHeader } from '../../../components/shared/view_content_header';
import {
NAV,
CUSTOMIZE_HEADER_TITLE,
CUSTOMIZE_HEADER_DESCRIPTION,
CUSTOMIZE_NAME_LABEL,
Expand All @@ -31,32 +32,36 @@ export const Customize: React.FC = () => {
};

return (
<form onSubmit={handleSubmit}>
<ViewContentHeader
title={CUSTOMIZE_HEADER_TITLE}
description={CUSTOMIZE_HEADER_DESCRIPTION}
/>
<ContentSection>
<EuiFormRow label={CUSTOMIZE_NAME_LABEL} fullWidth isInvalid={false}>
<EuiFlexGroup>
<EuiFlexItem>
<EuiFieldText
isInvalid={false}
required
value={orgNameInputValue}
data-test-subj="OrgNameInput"
onChange={(e) => onOrgNameInputChange(e.target.value)}
/>
</EuiFlexItem>
<EuiFlexItem />
</EuiFlexGroup>
</EuiFormRow>
<EuiFormRow>
<EuiButton color="primary" data-test-subj="SaveOrgNameButton" type="submit">
{CUSTOMIZE_NAME_BUTTON}
</EuiButton>
</EuiFormRow>
</ContentSection>
</form>
<WorkplaceSearchPageTemplate
pageChrome={[NAV.SETTINGS]}
pageHeader={{
pageTitle: CUSTOMIZE_HEADER_TITLE,
description: CUSTOMIZE_HEADER_DESCRIPTION,
}}
>
<form onSubmit={handleSubmit}>
<ContentSection>
<EuiFormRow label={CUSTOMIZE_NAME_LABEL} fullWidth isInvalid={false}>
<EuiFlexGroup>
<EuiFlexItem>
<EuiFieldText
isInvalid={false}
required
value={orgNameInputValue}
data-test-subj="OrgNameInput"
onChange={(e) => onOrgNameInputChange(e.target.value)}
/>
</EuiFlexItem>
<EuiFlexItem />
</EuiFlexGroup>
</EuiFormRow>
<EuiFormRow>
<EuiButton color="primary" data-test-subj="SaveOrgNameButton" type="submit">
{CUSTOMIZE_NAME_BUTTON}
</EuiButton>
</EuiFormRow>
</ContentSection>
</form>
</WorkplaceSearchPageTemplate>
);
};
7 changes: 4 additions & 3 deletions ...public/applications/workplace_search/views/settings/components/oauth_application.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import { shallow } from 'enzyme';

import { EuiModal, EuiForm } from '@elastic/eui';

import { getPageDescription } from '../../../../test_helpers';

import { CredentialItem } from '../../../components/shared/credential_item';
import { ViewContentHeader } from '../../../components/shared/view_content_header';
import { OAUTH_DESCRIPTION, REDIRECT_INSECURE_ERROR_TEXT } from '../../../constants';

import { OauthApplication } from './oauth_application';
Expand Down Expand Up @@ -77,7 +78,7 @@ describe('OauthApplication', () => {
it('handles form submission', () => {
const wrapper = shallow(<OauthApplication />);
const preventDefault = jest.fn();
wrapper.find('form').simulate('submit', { preventDefault });
wrapper.find(EuiForm).simulate('submit', { preventDefault });

expect(updateOauthApplication).toHaveBeenCalled();
});
Expand Down Expand Up @@ -127,7 +128,7 @@ describe('OauthApplication', () => {
});
const wrapper = shallow(<OauthApplication />);

expect(wrapper.find(ViewContentHeader).prop('description')).toEqual(OAUTH_DESCRIPTION);
expect(getPageDescription(wrapper)).toEqual(OAUTH_DESCRIPTION);
expect(wrapper.find('[data-test-subj="RedirectURIsRow"]').prop('error')).toEqual(
REDIRECT_INSECURE_ERROR_TEXT
);
Expand Down
Loading