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

[Enterprise Search] Added a shouldShowActiveForSubroutes option #83338

Merged
merged 3 commits into from
Nov 16, 2020
Merged

[Enterprise Search] Added a shouldShowActiveForSubroutes option #83338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6581bfa
Added a shouldShowActiveForSubroutes option
JasonStoltz Nov 12, 2020
6f25961
Reorganize tests
JasonStoltz Nov 16, 2020
2a3b6a8
Merge branch 'master' into sidenav-link-update
JasonStoltz Nov 16, 2020
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
24 changes: 24 additions & 0 deletions x-pack/plugins/enterprise_search/public/applications/shared/layout/side_nav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,30 @@ describe('SideNavLink', () => {
expect(wrapper.find('.enterpriseSearchNavLinks__item--isActive')).toHaveLength(1);
});

it("won't set an active class when route is a subroute of 'to'", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

[not a blocker] What are your thoughts on wrapping these two new tests in a describe('shouldShowActiveForSubroutes' () => { block for organization and swapping the order (testing the positive first and then confirming the negative)? no worries if not, it's super minor

Copy link
Member Author

@JasonStoltz JasonStoltz Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I'll update this. I have to merge master anyway.

(useLocation as jest.Mock).mockImplementationOnce(() => ({ pathname: '/documents/1234' }));

const wrapper = shallow(
<SideNavLink to="/documents" isRoot>
Link
</SideNavLink>
);

expect(wrapper.find('.enterpriseSearchNavLinks__item--isActive')).toHaveLength(0);
});

it('sets an active class if the current path is a subRoute of "to", and shouldShowActiveForSubroutes is true', () => {
(useLocation as jest.Mock).mockImplementationOnce(() => ({ pathname: '/documents/1234' }));

const wrapper = shallow(
<SideNavLink to="/documents" isRoot shouldShowActiveForSubroutes>
Link
</SideNavLink>
);

expect(wrapper.find('.enterpriseSearchNavLinks__item--isActive')).toHaveLength(1);
});

it('passes down custom classes and props', () => {
const wrapper = shallow(
<SideNavLink to="/" className="testing" data-test-subj="testing">
Expand Down
9 changes: 7 additions & 2 deletions x-pack/plugins/enterprise_search/public/applications/shared/layout/side_nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,17 @@ export const SideNav: React.FC<SideNavProps> = ({ product, children }) => {

interface SideNavLinkProps {
to: string;
shouldShowActiveForSubroutes?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

After rereading a few times this var name is really growing on me! It's very clear what it means and I was able to figure it out without having to ask for a screenshot (which I almost did at first haha) 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha. I wanted to, but it was late in the day and and I was trying to get out the door so I thought I'd roll the dice 🎲

isExternal?: boolean;
className?: string;
isRoot?: boolean;
subNav?: React.ReactNode;
}

export const SideNavLink: React.FC<SideNavLinkProps> = ({
isExternal,
to,
shouldShowActiveForSubroutes = false,
isExternal,
children,
className,
isRoot,
Expand All @@ -82,7 +84,10 @@ export const SideNavLink: React.FC<SideNavLinkProps> = ({

const { pathname } = useLocation();
const currentPath = stripTrailingSlash(pathname);
const isActive = currentPath === to || (isRoot && currentPath === '');
const isActive =
currentPath === to ||
(shouldShowActiveForSubroutes && currentPath.startsWith(to)) ||
(isRoot && currentPath === '');

const classes = classNames('enterpriseSearchNavLinks__item', className, {
'enterpriseSearchNavLinks__item--isActive': !isExternal && isActive, // eslint-disable-line @typescript-eslint/naming-convention
Expand Down