This repository was archived by the owner on Jan 16, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 74
refactor: convert Author component to hooks #150
Merged
priscilawebdev
merged 10 commits into
verdaccio:master
from
bighuggies:ah/author-component-hooks
Oct 6, 2019
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0ba12a1
refactor: convert Author component to hooks
bighuggies b55bc39
chore: remove unneeded fragment
bighuggies e97ac56
chore: rename and move test fixture
bighuggies 98dd66c
chore: import Author type instead of inferring it
bighuggies e4a39ce
Merge branch 'master' of github.com:verdaccio/ui into ah/author-compo…
bighuggies 88eab23
fix: type error in Author interface
bighuggies 029fccd
Merge branch 'master' into ah/author-component-hooks
juanpicado 0cf82ba
Merge branch 'master' of github.com:verdaccio/ui into ah/author-compo…
bighuggies 845d09c
Merge branch 'ah/author-component-hooks' of github.com:bighuggies/ver…
bighuggies 580d257
Merge branch 'master' into ah/author-component-hooks
juanpicado File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,30 +1,21 @@ | ||
| import React from 'react'; | ||
| import { mount } from 'enzyme'; | ||
| import Authors from './Author'; | ||
|
|
||
| const mockPackageMeta = jest.fn(() => ({ | ||
| latest: { | ||
| homepage: 'https://verdaccio.tld', | ||
| bugs: { | ||
| url: 'https://verdaccio.tld/bugs', | ||
| }, | ||
| dist: { | ||
| tarball: 'https://verdaccio.tld/download', | ||
| }, | ||
| }, | ||
| })); | ||
| import { DetailContext } from '../../pages/Version'; | ||
|
|
||
| jest.mock('../../pages/Version', () => ({ | ||
| DetailContextConsumer: component => { | ||
| return component.children({ packageMeta: mockPackageMeta() }); | ||
| }, | ||
| })); | ||
| import Authors from './Author'; | ||
|
|
||
| describe('<Author /> component', () => { | ||
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| const component = (packageMeta: React.ContextType<typeof DetailContext>['packageMeta']): JSX.Element => ( | ||
| <DetailContext.Provider value={{ packageMeta }}> | ||
| <Authors /> | ||
| </DetailContext.Provider> | ||
| ); | ||
|
|
||
| test('should render the component in default state', () => { | ||
| const packageMeta = { | ||
| latest: { | ||
|
|
@@ -36,13 +27,12 @@ describe('<Author /> component', () => { | |
| url: '', | ||
| avatar: 'https://www.gravatar.com/avatar/000000', | ||
| }, | ||
| dist: { fileCount: 0, unpackedSize: 0 }, | ||
| }, | ||
| _uplinks: {}, | ||
| }; | ||
|
|
||
| // @ts-ignore | ||
| mockPackageMeta.mockImplementation(() => packageMeta); | ||
|
|
||
| const wrapper = mount(<Authors />); | ||
| const wrapper = mount(component(packageMeta)); | ||
| expect(wrapper.html()).toMatchSnapshot(); | ||
| }); | ||
|
|
||
|
|
@@ -51,14 +41,13 @@ describe('<Author /> component', () => { | |
| latest: { | ||
| name: 'verdaccio', | ||
| version: '4.0.0', | ||
| dist: { fileCount: 0, unpackedSize: 0 }, | ||
| }, | ||
| _uplinks: {}, | ||
|
Comment on lines
+44
to
+46
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are not needed for the test but were missing before. The type error was hidden by using |
||
| }; | ||
|
|
||
| // @ts-ignore | ||
| mockPackageMeta.mockImplementation(() => packageMeta); | ||
|
|
||
| const wrapper = mount(<Authors />); | ||
| expect(wrapper.html()).toEqual(''); | ||
| const wrapper = mount(component(packageMeta)); | ||
| expect(wrapper.html()).toBeNull(); | ||
juanpicado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| test('should render the component when there is no author email', () => { | ||
|
|
@@ -71,13 +60,12 @@ describe('<Author /> component', () => { | |
| url: '', | ||
| avatar: 'https://www.gravatar.com/avatar/000000', | ||
| }, | ||
| dist: { fileCount: 0, unpackedSize: 0 }, | ||
| }, | ||
| _uplinks: {}, | ||
| }; | ||
|
|
||
| // @ts-ignore | ||
| mockPackageMeta.mockImplementation(() => packageMeta); | ||
|
|
||
| const wrapper = mount(<Authors />); | ||
| const wrapper = mount(component(packageMeta)); | ||
| expect(wrapper.html()).toMatchSnapshot(); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,58 +1,44 @@ | ||
| import React, { Component, ReactNode, ReactElement } from 'react'; | ||
| import React, { FC, useContext } from 'react'; | ||
|
|
||
| import Avatar from '@material-ui/core/Avatar'; | ||
| import List from '@material-ui/core/List'; | ||
|
|
||
| import { DetailContextConsumer } from '../../pages/Version'; | ||
| import { DetailContext } from '../../pages/Version'; | ||
| import { Heading, AuthorListItem, AuthorListItemText } from './styles'; | ||
| import { isEmail } from '../../utils/url'; | ||
|
|
||
| class Authors extends Component { | ||
| public render(): ReactElement<HTMLElement> { | ||
| return ( | ||
| <DetailContextConsumer> | ||
| {context => { | ||
| const { packageMeta } = context; | ||
|
|
||
| if (!packageMeta) { | ||
| return null; | ||
| } | ||
|
|
||
| return this.renderAuthor(packageMeta); | ||
| }} | ||
| </DetailContextConsumer> | ||
| ); | ||
| const Authors: FC = () => { | ||
| const { packageMeta } = useContext(DetailContext); | ||
|
|
||
| if (!packageMeta) { | ||
| return null; | ||
| } | ||
|
|
||
| public renderLinkForMail(email: string, avatarComponent: ReactNode, packageName: string, version: string): ReactElement<HTMLElement> | ReactNode { | ||
| if (!email || isEmail(email) === false) { | ||
| return avatarComponent; | ||
| } | ||
| const { author, name: packageName, version } = packageMeta.latest; | ||
|
|
||
| return ( | ||
| <a href={`mailto:${email}?subject=${packageName}@${version}`} target={'_top'}> | ||
| {avatarComponent} | ||
| </a> | ||
| ); | ||
| if (!author) { | ||
| return null; | ||
| } | ||
|
|
||
| public renderAuthor = ({ latest }) => { | ||
juanpicado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const { author, name: packageName, version } = latest; | ||
|
|
||
| if (!author) { | ||
| return null; | ||
| } | ||
|
|
||
| const avatarComponent = <Avatar alt={author.name} src={author.avatar} />; | ||
| return ( | ||
| <List subheader={<Heading variant={'subtitle1'}>{'Author'}</Heading>}> | ||
| <AuthorListItem button={true}> | ||
| {this.renderLinkForMail(author.email, avatarComponent, packageName, version)} | ||
| <AuthorListItemText primary={author.name} /> | ||
| </AuthorListItem> | ||
| </List> | ||
| ); | ||
| }; | ||
| } | ||
| const { email, name } = author; | ||
|
|
||
| const avatarComponent = <Avatar alt={author.name} src={author.avatar} />; | ||
|
|
||
| return ( | ||
| <List subheader={<Heading variant={'subtitle1'}>{'Author'}</Heading>}> | ||
| <AuthorListItem button={true}> | ||
| {!email || !isEmail(email) ? ( | ||
| <>{avatarComponent}</> | ||
bighuggies marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) : ( | ||
| <a href={`mailto:${email}?subject=${packageName}@${version}`} target={'_top'}> | ||
| {avatarComponent} | ||
| </a> | ||
| )} | ||
|
|
||
| <AuthorListItemText primary={name} /> | ||
| </AuthorListItem> | ||
| </List> | ||
| ); | ||
| }; | ||
|
|
||
| export default Authors; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, as it is a component, it must be capitalize and have a more descriptive name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also write this component outside of the describe<'Author .....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
component==>mockAuthorComponent. It has to be lower case since is a function. Other popular naming way might bewithAuthorComponent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm unfortunately I don't agree with you @juanpicado it returns an JSX element and according to the standards it should be capitalize...Please see that also Function Components are functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main difference is that this doesn't take props, you can't use it as a JSX element.
That's why I didn't consider it a component and why I used lowercase, it's more of a function that returns JSX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priscilawebdev any last comment here? or can we merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juanpicado no...We can merge :-)