refactor: convert Author component to hooks by bighuggies · Pull Request #150 · verdaccio/ui

bighuggies · 2019-10-02T22:36:23Z

Type: refactor

The following has been addressed in the PR: #116

Unit or Functional tests are included in the PR
Minor changes had to be made to the mocking in the existing tests to enable hooks. The tests are also more typesafe.

Description:

Convert <Author /> to a function component
Replace render prop context consumer with useContext hook
Removed some indirection in the JSX by inlining functions
Add missing types to packageMeta (is there a better way to do this?)

bighuggies · 2019-10-02T22:37:07Z

src/components/Author/Author.test.tsx

+        dist: { fileCount: 0, unpackedSize: 0 },
      },
+      _uplinks: {},


These are not needed for the test but were missing before. The type error was hidden by using jest.fn

src/components/Author/Author.tsx

src/components/Author/Author.test.tsx

bighuggies · 2019-10-02T22:48:34Z

~~Tests failing - I think it's a platform or version issue (I'm on Windows). Running yarn install && yarn test on master fails on my machine due to different generated classes.~~

Fixed by running the tests on MacOS

src/components/Author/Author.tsx

priscilawebdev · 2019-10-03T11:42:16Z

src/components/Author/Author.test.tsx

    jest.resetAllMocks();
  });

+  const component = (packageMeta: React.ContextType<typeof DetailContext>['packageMeta']): JSX.Element => (


Hmmm, as it is a component, it must be capitalize and have a more descriptive name

I would also write this component outside of the describe<'Author .....

component ==> mockAuthorComponent . It has to be lower case since is a function. Other popular naming way might be withAuthorComponent.

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

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.

@priscilawebdev any last comment here? or can we merge?

@juanpicado no...We can merge :-)

types/packageMeta.ts

src/components/Package/Package.tsx

priscilawebdev · 2019-10-03T11:49:07Z

@bighuggies PR looks great! Thank you so much 👏, it needs just some minor changes..please check my comments.

…nent-hooks

codecov-io · 2019-10-04T16:26:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a365ec5). Click here to learn what that means.
The diff coverage is 90.9%.

@@            Coverage Diff            @@
##             master     #150   +/-   ##
=========================================
  Coverage          ?   82.97%           
=========================================
  Files             ?       97           
  Lines             ?      928           
  Branches          ?      163           
=========================================
  Hits              ?      770           
  Misses            ?      141           
  Partials          ?       17

Impacted Files	Coverage Δ
src/components/Package/Package.tsx	`100% <ø> (ø)`
src/components/Author/Author.tsx	`90.9% <90.9%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a365ec5...580d257. Read the comment docs.

juanpicado

hi @bighuggies , after check the changes, seems something is broken.

it should looks like

or you can double check here

https://registry.verdaccio.org/-/web/detail/jquery/v/2.2.1

I haven't dug enough to see the reason, but let me know whether you have troubles to solve it. I can help.

…nent-hooks

…daccio-ui into ah/author-component-hooks

bighuggies · 2019-10-05T16:44:52Z

That seems to be an issue on master as well.

A quick git bisect finds this change is probably responsible: f84fd79?w=1#diff-8b0edc064390f472a826cacf9ea97dc4R6

I will open a separate PR to fix it 👍

EDIT: #160

juanpicado · 2019-10-05T18:14:23Z

Thanks @bighuggies

juanpicado

Great job @bighuggies !

priscilawebdev · 2019-10-06T13:42:59Z

thank you @bighuggies... yeah the Tooltip import was wrong.. I need to check why the tests passed...good job!

bighuggies commented Oct 2, 2019

View reviewed changes

src/components/Author/Author.tsx Show resolved Hide resolved

bighuggies commented Oct 2, 2019

View reviewed changes

src/components/Author/Author.test.tsx Show resolved Hide resolved

refactor: convert Author component to hooks

0ba12a1

bighuggies force-pushed the ah/author-component-hooks branch from 834b0e4 to 0ba12a1 Compare October 3, 2019 08:30

priscilawebdev suggested changes Oct 3, 2019

View reviewed changes

src/components/Author/Author.tsx Outdated Show resolved Hide resolved

priscilawebdev suggested changes Oct 3, 2019

View reviewed changes

priscilawebdev added hacktoberfest 🎃 good first issue Good for newcomers PR: needs review labels Oct 3, 2019

bighuggies added 3 commits October 3, 2019 20:39

chore: remove unneeded fragment

b55bc39

chore: rename and move test fixture

e97ac56

chore: import Author type instead of inferring it

98dd66c

bighuggies requested review from juanpicado and priscilawebdev October 3, 2019 20:32

bighuggies added 2 commits October 3, 2019 21:35

Merge branch 'master' of github.com:verdaccio/ui into ah/author-compo…

e4a39ce

…nent-hooks

fix: type error in Author interface

88eab23

bighuggies mentioned this pull request Oct 3, 2019

refactor: convert Dist component to hooks #156

Merged

Merge branch 'master' into ah/author-component-hooks

029fccd

juanpicado suggested changes Oct 4, 2019

View reviewed changes

bighuggies added 2 commits October 5, 2019 17:10

Merge branch 'master' of github.com:verdaccio/ui into ah/author-compo…

0cf82ba

…nent-hooks

Merge branch 'ah/author-component-hooks' of github.com:bighuggies/ver…

845d09c

…daccio-ui into ah/author-component-hooks

bighuggies mentioned this pull request Oct 5, 2019

fix: incorrect Tooltip import in avatar component #160

Merged

Merge branch 'master' into ah/author-component-hooks

580d257

juanpicado approved these changes Oct 5, 2019

View reviewed changes

priscilawebdev approved these changes Oct 6, 2019

View reviewed changes

priscilawebdev merged commit d2f1f1c into verdaccio:master Oct 6, 2019

bighuggies mentioned this pull request Oct 28, 2019

refactor: convert Engine component to hooks #233

Merged

bighuggies deleted the ah/author-component-hooks branch October 29, 2019 21:51

Conversation

bighuggies commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bighuggies commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

priscilawebdev commented Oct 3, 2019

Uh oh!

codecov-io commented Oct 4, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

juanpicado left a comment

Choose a reason for hiding this comment

Uh oh!

bighuggies commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juanpicado commented Oct 5, 2019

Uh oh!

juanpicado left a comment

Choose a reason for hiding this comment

Uh oh!

priscilawebdev commented Oct 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bighuggies commented Oct 2, 2019 •

edited

Loading

bighuggies commented Oct 2, 2019 •

edited

Loading

codecov-io commented Oct 4, 2019 •

edited by codecov bot

Loading

bighuggies commented Oct 5, 2019 •

edited

Loading