-
Notifications
You must be signed in to change notification settings - Fork 2
Review #3
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
base: review
Are you sure you want to change the base?
Conversation
add babelrc file
Repo card
Add fetching user details, avatar and repos
merge staging to main
change title in the index.html
@@ -10,38 +11,56 @@ This app was created by [@roselynle](https://github.com/roselynle), [@emmanuel-s | |||
|
|||
### Usage |
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.
clear usage instructions
- Run `npm run dev` which should start up the webpage on localhost:8080/ | ||
- Run `npm run test` to start up the unit tests | ||
|
||
Deployed site can be accessed here: https://your-repo-story.netlify.app/ | ||
|
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.
nice that you deployed the site
@@ -34,6 +34,7 @@ | |||
}, | |||
"dependencies": { | |||
"react": "^17.0.2", | |||
"react-dom": "^17.0.2" | |||
"react-dom": "^17.0.2", | |||
"react-icons": "^4.2.0" |
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.
cool library
margin-top: 284px; | ||
z-index: -1; | ||
} | ||
|
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.
really cool timeline - nice that you made this with just css
<SearchForm setUsername={setUsername} /> | ||
<div id="text-container"> | ||
<h3 aria-label="Form" id="Form"> |
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.
not sure if this should have a different label?
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.
ooh yeh the aria-label="form" should probs be with your form element and maybe named something to describe what the form does e.g "search"
<div className="forks"> | ||
<BiGitRepoForked /><span>{ repo.forks_count}</span> | ||
</div> | ||
<div className="stars"> | ||
<BiStar /><span>{ repo.stargazers_count}</span> | ||
</div> | ||
<div className="watchers"> | ||
<BsEyeFill /><span>{ repo.watchers_count}</span> | ||
</div> |
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.
could base these off a reusable component?
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.
ah nice idea, didn't think of that!
src/components/RepoList/index.js
Outdated
getUser(); | ||
}, [username]); | ||
|
||
const reposList = Array.isArray(repos) && repos.map(repo => <RepoCard key={repo.id} repo={repo} />); |
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.
like the approach for handling this
) | ||
} | ||
|
||
export default UserCard; |
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.
nice component - good idea to fetch the user data as well as repos and display that info
width: 20rem; | ||
height: 2rem; | ||
border: 4px solid black; | ||
} |
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.
when the user goes to click submit button the input shrinks in size as it loses focus, which shifts the button away from where the user is clicking, and this seems to fire before the click on the button registers - I think just make sure the submit button is still in the location it was when user clicked !
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.
Cool project. Really like the timeline and nice that you were able to make that with only CSS. Just noticed that one UI bug with the text input resizing and user having to click twice on the button.
Interesting to see how you handled some of the common problems with react like making sure components aren't trying to render without data being there etc.
I checked out the test suite, nice coverage there. It was giving me an error in the SearchForm tests, you guys may have sorted it but adding this at the start fixed that for me:
let setUsernameMock;
beforeEach(() => {
getResultMock = jest.fn();
render(<SearchForm setUsername={setUsernameMock}/>);
});
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.
guyssss well done, this looks AMAZING! esp love the timeline its such a simple yet creative idea to display repos. Maybe you could make it in a time relative order for future (just seen thats in your future features!!). Anyways love itttttt, the color scheme is beaut too 💖
|
||
Are you as popular on GitHub as you think you are? Well now you can find out! Our repo tracker allows you to search for a particular GitHub user and retrieve their list of repos, including stats such as stargazers and forks | ||
Are you as popular on GitHub as you think you are? Well now you can find out! Our repo tracker allows you to search for a particular GitHub user and retrieve their list of repos, including data such as description and the number of forks, stars and watchers. A link to each repo is also auto-generated. |
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.
love that opening line!
- [x] Your app should make use of React | ||
- [x] Your repo tracker should have an input for users to give their GitHub username | ||
- [x] After submitting their username, use the GitHub API to retrieve that user's list of repos | ||
- [x] When selecting a repo, a User should be shown some data about that repo eg. issue count, stargazers, forks etc. | ||
|
||
## Wins & Challenges |
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.
this is great! love to see your workflow and that you're recognising many wins!
@@ -4,7 +4,8 @@ | |||
<meta charset="UTF-8" /> | |||
<meta http-equiv="X-UA-Compatible" content="IE=edge" /> | |||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> |
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.
favicon! 🙌🏼
<SearchForm setUsername={setUsername} /> | ||
<div id="text-container"> | ||
<h3 aria-label="Form" id="Form"> |
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.
ooh yeh the aria-label="form" should probs be with your form element and maybe named something to describe what the form does e.g "search"
</div> | ||
<div className="repo-details"> | ||
<div className="forks"> | ||
<BiGitRepoForked /><span>{ repo.forks_count}</span> |
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.
icons look amazing!!
src/components/RepoList/index.js
Outdated
try { | ||
const response = await fetch(`${API_URL}/users/${username}`); | ||
const data = await response.json(); | ||
if (data.documentation_url === "https://docs.github.com/rest/reference/users#get-a-user") {alert("Error: not a valid user")} |
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.
nice error handling!
No description provided.