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

[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 1 commit
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
Member 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
Member 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
38 changes: 30 additions & 8 deletions .../public/applications/workplace_search/views/settings/components/settings_sub_nav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,40 @@
* 2.0.
*/

import React from 'react';
jest.mock('../../../../shared/layout', () => ({
generateNavLink: jest.fn(({ to }) => ({ href: to })),
}));

import { shallow } from 'enzyme';
import { mockUseRouteMatch } from '../../../../__mocks__/react_router';

import { SideNavLink } from '../../../../shared/layout';
import { useSettingsSubNav } from './settings_sub_nav';

import { SettingsSubNav } from './settings_sub_nav';
describe('useSettingsSubNav', () => {
it('returns an array of side nav items when on the /settings path', () => {
mockUseRouteMatch.mockReturnValueOnce(true);

describe('SettingsSubNav', () => {
it('renders', () => {
const wrapper = shallow(<SettingsSubNav />);
expect(useSettingsSubNav()).toEqual([
{
id: 'settingsCustomize',
name: 'Customize',
href: '/settings/customize',
},
{
id: 'settingsConnectors',
name: 'Content source connectors',
href: '/settings/connectors',
},
{
id: 'settingsOAuth',
name: 'OAuth application',
href: '/settings/oauth',
},
]);
});

it('returns undefined when not on the /settings path', () => {
mockUseRouteMatch.mockReturnValueOnce(false);

expect(wrapper.find(SideNavLink)).toHaveLength(3);
expect(useSettingsSubNav()).toEqual(undefined);
});
});
40 changes: 29 additions & 11 deletions ...earch/public/applications/workplace_search/views/settings/components/settings_sub_nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,40 @@
* 2.0.
*/

import React from 'react';
import { useRouteMatch } from 'react-router-dom';

import { SideNavLink } from '../../../../shared/layout';
import { EuiSideNavItemType } from '@elastic/eui';

import { generateNavLink } from '../../../../shared/layout';
import { NAV } from '../../../constants';
import {
ORG_SETTINGS_PATH,
ORG_SETTINGS_CUSTOMIZE_PATH,
ORG_SETTINGS_CONNECTORS_PATH,
ORG_SETTINGS_OAUTH_APPLICATION_PATH,
} from '../../../routes';

export const SettingsSubNav: React.FC = () => (
<>
<SideNavLink to={ORG_SETTINGS_CUSTOMIZE_PATH}>{NAV.SETTINGS_CUSTOMIZE}</SideNavLink>
<SideNavLink to={ORG_SETTINGS_CONNECTORS_PATH}>
{NAV.SETTINGS_SOURCE_PRIORITIZATION}
</SideNavLink>
<SideNavLink to={ORG_SETTINGS_OAUTH_APPLICATION_PATH}>{NAV.SETTINGS_OAUTH}</SideNavLink>
</>
);
export const useSettingsSubNav = () => {
const isSettingsPage = !!useRouteMatch(ORG_SETTINGS_PATH);
if (!isSettingsPage) return undefined;

const navItems: Array<EuiSideNavItemType<unknown>> = [
{
id: 'settingsCustomize',
name: NAV.SETTINGS_CUSTOMIZE,
...generateNavLink({ to: ORG_SETTINGS_CUSTOMIZE_PATH }),
},
{
id: 'settingsConnectors',
name: NAV.SETTINGS_SOURCE_PRIORITIZATION,
...generateNavLink({ to: ORG_SETTINGS_CONNECTORS_PATH, shouldShowActiveForSubroutes: true }),
},
{
id: 'settingsOAuth',
name: NAV.SETTINGS_OAUTH,
...generateNavLink({ to: ORG_SETTINGS_OAUTH_APPLICATION_PATH }),
},
];

return navItems;
};