Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add Author Github homepage url if exists (fixes #124) #145

Merged
merged 5 commits into from
Oct 13, 2019

Conversation

anku255
Copy link
Contributor

@anku255 anku255 commented Oct 4, 2019

Hi @kefranabg !

I am referring Author's Github Website page URL as Author Homepage in the codebase. Please let me know if you want me to change this.

Some points that will help during the code review:

  • Screenshot of the README

image

  • I am using node-fetch library to make the get request.

  • I have added some tests but I guess I need to add a few more to get 100% code coverage. I will do that soon.

  • I needed to update the snapshots to make the tests pass.

Please take a look and let me know what you think.
Thank you.

@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #145 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #145   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     27    +1     
  Lines         211    226   +15     
  Branches       23     26    +3     
=====================================
+ Hits          211    226   +15
Impacted Files Coverage Δ
src/questions/index.js 100% <ø> (ø) ⬆️
src/project-infos.js 100% <100%> (ø) ⬆️
src/utils.js 100% <100%> (ø) ⬆️
src/questions/author-website.js 100% <100%> (ø)

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 5b8bc08...c6bc952. Read the comment docs.

@kefranabg
Copy link
Owner

Hi @anku255,

Sorry for the delay... I've been busy these last days. I'll do a review very soon.
Looks great !

@anku255
Copy link
Contributor Author

anku255 commented Oct 13, 2019

Hi @anku255,

Sorry for the delay... I've been busy these last days. I'll do a review very soon.
Looks great !

Hi!!

No Problem! Please let me know if you need help with anything :)

kefranabg
kefranabg previously approved these changes Oct 13, 2019
Copy link
Owner

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

@anku255,

First thanks for the PR, this is great work 💪

I just added a few changes:

  • Renaming Homepage to Website
  • Move Website question just after Github username question
  • Made an improvement on the website question: now, if the user uses a github username different that the one that has been proposed by readme-md-generator, it will dynamically fetch the website url using the new username (and not the one that had been found before the question process starts. See these changes)

Please let me know if you're ok with my changes. Thanks

@anku255
Copy link
Contributor Author

anku255 commented Oct 13, 2019

@kefranabg Thank you for the kind words. Your change looks great. So thoughtful of you to fetch author's website dynamically if user enters a different user name.

This was by far my biggest PR ( considering the number of files I had to change). It's been great working on this project and I would like to continue doing so in the future. ☺️

Copy link
Owner

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

Awesome ! Feel free to contribute again or open new issues if you have features ideas 👍

@kefranabg kefranabg merged commit 541d6e5 into kefranabg:master Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants