Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

refactor: convert Engine component to hooks #233

Merged
merged 7 commits into from
Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
71 changes: 30 additions & 41 deletions src/components/Engines/Engines.test.tsx
Original file line number Diff line number Diff line change
@@ -1,71 +1,60 @@
import React from 'react';
import { mount } from 'enzyme';
priscilawebdev marked this conversation as resolved.
Show resolved Hide resolved

import { DetailContext } from '../../pages/Version';
import { PackageMetaInterface } from '../../../types/packageMeta';

import Engine from './Engines';

jest.mock('./img/node.png', () => '');
jest.mock('../Install/img/npm.svg', () => '');

const mockPackageMeta: jest.Mock = jest.fn(() => ({
const withEngineComponent = (packageMeta: PackageMetaInterface): JSX.Element => (
priscilawebdev marked this conversation as resolved.
Show resolved Hide resolved
<DetailContext.Provider value={{ packageMeta }}>
<Engine />
</DetailContext.Provider>
);

const mockPackageMeta = (): PackageMetaInterface => ({
bighuggies marked this conversation as resolved.
Show resolved Hide resolved
latest: {
homepage: 'https://verdaccio.tld',
bugs: {
url: 'https://verdaccio.tld/bugs',
},
name: 'verdaccio',
version: '0.0.0',
dist: {
tarball: 'https://verdaccio.tld/download',
fileCount: 1,
unpackedSize: 1,
},
},
}));

jest.mock('../../pages/Version', () => ({
DetailContextConsumer: component => {
return component.children({ packageMeta: mockPackageMeta() });
},
}));
_uplinks: {},
});

describe('<Engines /> component', () => {
beforeEach(() => {
jest.resetAllMocks();
});

test('should render the component in default state', () => {
const packageMeta = {
latest: {
engines: {
node: '>= 0.1.98',
npm: '>3',
},
},
};
const packageMeta = mockPackageMeta();

mockPackageMeta.mockImplementation(() => packageMeta);
packageMeta.latest.engines = {
node: '>= 0.1.98',
npm: '>3',
};

const wrapper = mount(<Engine />);
const wrapper = mount(withEngineComponent(packageMeta));
expect(wrapper.html()).toMatchSnapshot();
});

test('should render the component when there is no engine key in package meta', () => {
const packageMeta = {
latest: {},
};
const packageMeta = mockPackageMeta();

mockPackageMeta.mockImplementation(() => packageMeta);
delete packageMeta.latest.engines;
bighuggies marked this conversation as resolved.
Show resolved Hide resolved

const wrapper = mount(<Engine />);
expect(wrapper.html()).toEqual('');
const wrapper = mount(withEngineComponent(packageMeta));
expect(wrapper.html()).toBeNull();
});

test('should render the component when there is no keys in engine in package meta', () => {
const packageMeta = {
latest: {
engines: {},
},
};
const packageMeta = mockPackageMeta();

mockPackageMeta.mockImplementation(() => packageMeta);
packageMeta.latest.engines = {};

const wrapper = mount(<Engine />);
expect(wrapper.html()).toEqual('');
const wrapper = mount(withEngineComponent(packageMeta));
expect(wrapper.html()).toBeNull();
});
});
94 changes: 36 additions & 58 deletions src/components/Engines/Engines.tsx
Original file line number Diff line number Diff line change
@@ -1,73 +1,51 @@
import React, { Component, ReactElement } from 'react';
import React, { useContext } from 'react';
import Grid from '@material-ui/core/Grid';
import ListItemText from '@material-ui/core/ListItemText';

import { VersionPageConsumerProps, DetailContextConsumer } from '../../pages/Version';
import { DetailContext } from '../../pages/Version';
import Avatar from '../../muiComponents/Avatar';
import List from '../../muiComponents/List';
import npm from '../Install/img/npm.svg';

import { StyledText, EngineListItem } from './styles';
// @ts-ignore
import node from './img/node.png';

const ICONS = {
'node-JS': <Avatar src={node} />,
'NPM-version': <Avatar src={npm} />,
};

class Engine extends Component {
public render(): ReactElement<HTMLElement> {
return (
<DetailContextConsumer>
{context => {
return this.renderEngine(context as VersionPageConsumerProps);
}}
</DetailContextConsumer>
);
}

private renderEngine = ({ packageMeta }): ReactElement<HTMLElement> | null => {
const { engines } = packageMeta.latest;
if (!engines) {
return null;
}
const Engine: React.FC = () => {
const { packageMeta } = useContext(DetailContext);

const engineDict = {
'node-JS': engines.node,
'NPM-version': engines.npm,
};
const engines = packageMeta && packageMeta.latest && packageMeta.latest.engines;
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 saw that TypeScript 3.7 is specified in package.json, but ?. doesn't seem to be supported by the tooling yet :(

Copy link
Member

Choose a reason for hiding this comment

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

That's odd, I did not tried yet. I'll double check.

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 think it was prettier that was giving me issues, 1.19 with support for 3.7 isn't released yet: https://github.com/prettier/prettier/blob/master/CHANGELOG.unreleased.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? 😞 I didn't test the new features yet, but I thought we could already use it ... so let's update prettier :D


const accumulator: React.ReactNode[] = [];
const items = Object.keys(engineDict).reduce((markup, text, key) => {
const heading = engineDict[text];
if (heading) {
markup.push(
<Grid item={true} key={key} xs={6}>
{this.renderListItems(heading, text)}
</Grid>
);
}
return markup;
}, accumulator);

if (items.length < 1) {
return null;
}

return <Grid container={true}>{items}</Grid>;
};
if (!engines || (!engines.node && !engines.npm)) {
return null;
}

private renderListItems = (heading: string, text: string) => {
return (
<List subheader={<StyledText variant={'subtitle1'}>{text.split('-').join(' ')}</StyledText>}>
<EngineListItem button={true}>
{ICONS[text]}
<ListItemText primary={heading} />
</EngineListItem>
</List>
);
};
}
/* eslint-disable react/jsx-max-depth */
bighuggies marked this conversation as resolved.
Show resolved Hide resolved
return (
<Grid container={true}>
{engines.node && (
priscilawebdev marked this conversation as resolved.
Show resolved Hide resolved
<Grid item={true} xs={6}>
<List subheader={<StyledText variant={'subtitle1'}>{'node JS'}</StyledText>}>
<EngineListItem button={true}>
Copy link
Member

Choose a reason for hiding this comment

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

You can remove button={true}

<Avatar src={node} />
<ListItemText primary={engines.node} />
</EngineListItem>
</List>
</Grid>
)}

{engines.npm && (
<Grid item={true} xs={6}>
<List subheader={<StyledText variant={'subtitle1'}>{'NPM version'}</StyledText>}>
<EngineListItem button={true}>
Copy link
Member

Choose a reason for hiding this comment

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

Same here, You can remove button={true}

Copy link
Member

Choose a reason for hiding this comment

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

This should be enforced by eslint, I'd suggest let it go and add the plugin or rule and change all places at a time.

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 think @ayusharma is referring to the fact that the EngineListItem styled component doesn't accept a button prop.

I left it because removing it breaks the snapshot tests, which I consider bad practice when doing a refactor like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi all! I just checked and the ListItem component from Material UI does accepted the prop button..we should change it if we don't want to use the ButtonBase only :
image

So IMO it's fine if you leave like it is now...we will check all cases in our design system

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@priscilawebdev priscilawebdev Oct 31, 2019

Choose a reason for hiding this comment

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

regarding this lint rule, I believe we should discuss about it. I find this rule very annoying ... I also prefer to write button only

<Avatar src={npm} />
<ListItemText primary={engines.npm} />
</EngineListItem>
</List>
</Grid>
)}
</Grid>
);
/* eslint-enable react/jsx-max-depth */
};

export default Engine;
4 changes: 4 additions & 0 deletions types/packageMeta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ export interface PackageMetaInterface {
fileCount: number;
unpackedSize: number;
};
engines?: {
node?: string;
npm?: string;
};
license?: Partial<LicenseInterface> | string;
version: string;
};
Expand Down