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

Refactor/116 RegistryInfoContent is converted to functional component#229

Merged
juanpicado merged 18 commits intoverdaccio:masterfrom
alfonsoar:refactor/116_registry_info_content_is_converted_to_functional_component
Nov 2, 2019
Merged

Refactor/116 RegistryInfoContent is converted to functional component#229
juanpicado merged 18 commits intoverdaccio:masterfrom
alfonsoar:refactor/116_registry_info_content_is_converted_to_functional_component

Conversation

@alfonsoar
Copy link
Copy Markdown
Member

Type: Refactor

The following has been addressed in the PR:

  • There is a related issue? #116
  • Unit or Functional tests are included in the PR: Yes

Description:

Refactor RegistryInfoContent compoenent to functional

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 27, 2019

Codecov Report

Merging #229 into master will increase coverage by 0.04%.
The diff coverage is 91.3%.

@@            Coverage Diff            @@
##           master    #229      +/-   ##
=========================================
+ Coverage   88.85%   88.9%   +0.04%     
=========================================
  Files         139     139              
  Lines         915     919       +4     
  Branches      142     160      +18     
=========================================
+ Hits          813     817       +4     
+ Misses         91      87       -4     
- Partials       11      15       +4
Impacted Files Coverage Δ
src/components/PackageList/PackageList.tsx 100% <100%> (ø) ⬆️
...onents/RegistryInfoContent/RegistryInfoContent.tsx 90% <88.88%> (+3.33%) ⬆️
src/utils/styles/media.ts 81.81% <0%> (ø) ⬆️
src/components/DetailSidebar/DetailSidebar.tsx 85% <0%> (ø) ⬆️

@alfonsoar
Copy link
Copy Markdown
Member Author

@priscilawebdev @juanpicado please review.

@juanpicado juanpicado self-requested a review October 28, 2019 03:51
juanpicado
juanpicado previously approved these changes Oct 28, 2019
Copy link
Copy Markdown
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

LGTM

@priscilawebdev
Copy link
Copy Markdown
Contributor

priscilawebdev commented Oct 28, 2019

@agar23 thank you very much for your contribution 🙂👏I would apply just a few minor changes ... please check my comments :-)

@alfonsoar
Copy link
Copy Markdown
Member Author

@agar23 thank you very much for your contribution 🙂👏I would apply just a few minor changes ... please check my comments :-)

np, I addressed most of the comments. but had a couple of questions/comments I left.

Copy link
Copy Markdown
Contributor

@priscilawebdev priscilawebdev left a comment

Choose a reason for hiding this comment

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

.

return packages.map(({ name, version, description, time, keywords, dist, homepage, bugs, author, license }, i) => {
// TODO: move format license to API side.
const license = formatLicense(pkg.license);
const _license = formatLicense(license);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please why did you add _ ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we cant name this variable license the props already have a prop named license I presume this why the component was not destructuring the props before. this way we could call pkg.license

);
}
const RegistryInfoContent: React.FC<Props> = props => {
const [tabPosition, setTabPosition] = useState<State['tabPosition']>(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [tabPosition, setTabPosition] = useState<State['tabPosition']>(0);
const [tabPosition, setTabPosition] = useState(0);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

as discussed I think it should be ok to be explicit here

@priscilawebdev
Copy link
Copy Markdown
Contributor

@agar23 almost there :-)

@juanpicado
Copy link
Copy Markdown
Member

@agar23 great job, there are one conflict and seems linting is failing :) looking forward to merge it soon :)

alfonsoar and others added 3 commits October 31, 2019 18:16
…tional_component' of github.com:agar23/ui into refactor/116_registry_info_content_is_converted_to_functional_component
Copy link
Copy Markdown
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

great @agar23 🚀 🚀 🚀 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants