Convert SideNav to Typescript

[rlesniak]

Summary

Closes: #2659

Checklist

• [ ] Check against all themes for compatibility in both light and dark modes
• [ ] Checked in mobile
• [ ] Checked in IE11 and Firefox
• Props have proper autodocs
• [ ] Added documentation examples
• [ ] Added or updated jest tests
• [ ] Checked for breaking changes and labeled appropriately
• [ ] Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

Comment

I had to remove Jest from lint-staged due to The --findRelatedTests option requires file paths to be specified. error. I dont know what is wrong :(

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[chandlerprall]

Thank you very much for taking this conversion! This looks great overall - I've left a few small requests around organization but there are two outstanding issues I've not addressed in comments:

Item documentation

Removing the proptype & comment block for items from EuiSideNavProps also removed it from the Props tab for this component. Your changes are correct, but it leaves a documentation gap. We have addressed this in other places with a hack that introduces the sub-type (e.g. Item) as a linkable entry in the Props tab. For example, Data Grid lists other shapes such as EuiDataGridColumn and EuiDataGridColumnVisibility. This work-around is setup in datagrid_example.js:306 and datagrid/props.tsx

If this makes sense, please follow the same pattern to include and link to EuiSideNavItem within the Props tab. If it doesn't make sense I'm happy to throw it together.

renderItem

The ...rest in side_nav_item.tsx is spread onto the renderItem component, which creates a very interesting interaction of props. I tried some quick changes but didn't get anything compilable and will keep working on this. As an example, this should be valid:

    <EuiSideNavItem
      renderItem={({ href, onClick, className, children, extraValue }) => {
        return (
          <a
            className={className}
            href={href}
            onClick={onClick}
            data-extra={extraValue}>
            {children}
          </a>
        );
      }}
      extraValue={8}>
      Test
    </EuiSideNavItem>

[chandlerprall]

I had to remove Jest from lint-staged due to The --findRelatedTests option requires file paths to be specified. error. I dont know what is wrong :(

Sorry, I missed your comment here on my first pass! (thanks to @myasonik for pointing it out). It looks like our script to run only affected tests doesn't catch renamed files. I'll investigate this separately.

Ironically, your modification to package.json causes the check to pass, except that it also disables the check.

[rlesniak]

@chandlerprall I have a problem with extracting props to new file side_nav_types.ts. I did it exactly how it is done in DataGrid but doesnt show in Docs UI. I created apropriate files, import them to side_nav_example.js, passed to props:{} object and nothing, it doesnt show up in docs. However, when I do smth like this:

import React, { FunctionComponent } from 'react';
// import { EuiSideNavItemType } from '../../../../src/components/side_nav/side_nav_types'; // That doesnt work
import { EuiDataGridColumn } from '../../../../src/components/datagrid/data_grid_types';

export const SideNavItem: FunctionComponent<EuiDataGridColumn> = () => <div />;

it shows in Docs. What I am doing wrong?

EDIT: Look at the latest commit

[chandlerprall]

I checked your changes locally, the docs for EuiSideNavItem are coming through for me. Sometimes our TypeScript->PropTypes->Docs generation gets out of sync with the dependency tree and the cache needs to be deleted (rm -rf .cache-loader/ in EUI repo) before it updates.

Looking at the other changes now.

[chandlerprall]

Changes look good, thanks! Added one new comment on the docs link to EuiSideNavItem, and there is still the change to package.json to revert. This also needs some Jest snapshot updates - they can be applied automatically by running yarn test-unit -u in the EUI repo and committing the resulting changes.

Otherwise this is looking good except for the tricky renderItem, which I'll make a full pass against tomorrow.

[chandlerprall]

EuiSideNavItem instead of EuiSideNavItemType (matching the parameter name from https://github.com/elastic/eui/pull/2818/files#diff-e5a3e909f17d33572c3c3cc48dbb6ae3R53)

[rlesniak]

There will be a name conflict between component <-> type there :/

[chandlerprall]

This change would only be in this comment; #EuiSideNavItem needs to match the rendered heading on the Props tab,

[rlesniak]

I updated regarding to your comments. Waiting for your move with renderItem :)

[chandlerprall]

@rlesniak I created a PR (rlesniak#1) against your branch with my type changes.

/cc @thompsongl if you wouldn't mind glancing through that PR

[chandlerprall]

Changes LGTM! Pulled and tested functionality and autodocs locally. Also built euid.d.ts and confirmed expected types are present. Will merge on green CI.

[chandlerprall]

jenkins test this

[chandlerprall]

@rlesniak thank you for providing this conversion!