-
Notifications
You must be signed in to change notification settings - Fork 1
Feedback #12
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: feedback
Are you sure you want to change the base?
Feedback #12
Conversation
added base of RepoCard
test branch
get Toms work
Tom changes
add favicon
|
✔️ Deploy Preview for repo-getter ready! 🔨 Explore the source changes: a5102d6 🔍 Inspect the deploy log: https://app.netlify.com/sites/repo-getter/deploys/6113e59c9f78310008569b49 😎 Browse the preview: https://deploy-preview-12--repo-getter.netlify.app |
zakgogi
left a comment
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 overall, really like the regexp matching stuff and the design in general, was hard to find something to criticise, good work
| setError(false); | ||
| window.location.pathname = `/${username}`; | ||
| } catch (error) { | ||
| setError('Unable to find GitHub user, please try again.'); |
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 that you handled the error here, when we did it we checked on the next page so we didn't have to fetch more than once but this is nicer I think
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.
Realised that because the error is handled here, if I just type an invalid username in the top bar it loads forever rather than using an error
| @@ -0,0 +1,33 @@ | |||
| import { screen } from '@testing-library/react'; | |||
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 added some tests
| @@ -0,0 +1,21 @@ | |||
| import React, { useState } from 'react'; | |||
| import { TextField } from '@material-ui/core'; | |||
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've never used material-ui before, looks cool
| setLoading(false); | ||
| }, []) | ||
|
|
||
| function closeModal(){ |
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 that you used Modals, we just redirected to another page which was much less elegant
| import './style.css' | ||
|
|
||
| const Repos = () => { | ||
| const { username } = useParams(); |
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 solution
| <Route path = '/:username'> | ||
| <Repos/> | ||
| </Route> | ||
| <Route path = '/'> |
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.
Feel like you potentially might need an exact in here?
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.
our plan was to send them back to the for in case of error
| @@ -0,0 +1,34 @@ | |||
| # Changelog | |||
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 readme, like that you used a gif
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 used ScreenToGif for Windowsx64 ;)
|
In agreement with Zak |
No description provided.