-
Notifications
You must be signed in to change notification settings - Fork 0
Push branch with codebase for code review #1
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,35 @@ | ||
| # tsh_github | ||
| github tsh app | ||
| # GH-Browser | ||
|
|
||
| A browser for Github users. Entering github username will display user's details and history of his public events. | ||
|
|
||
| ## Installation | ||
|
|
||
| Install npm packages: | ||
| ``` | ||
| yarn | ||
| ``` | ||
|
|
||
|
|
||
| ## Develop locally | ||
|
|
||
| 1. Run: | ||
|
|
||
| ``` | ||
| yarn start | ||
| ``` | ||
|
|
||
| 2. In your browser, navigate to: [http://localhost:2000/](http://localhost:2000/) | ||
|
|
||
| ## Build production-ready app | ||
|
|
||
| ``` | ||
| yarn build | ||
| ``` | ||
|
|
||
|
|
||
| # Technical details | ||
|
|
||
| * Bundler: [Webpack](https://webpack.js.org/) | ||
| * DOM Manipulation: [Cash-DOM](https://github.com/kenwheeler/cash) | ||
| * CSS styling: [Bulma CSS](https://bulma.io/) | ||
| * [Github API](https://api.github.com/) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| # TSH recruitment task | ||
|
|
||
| Hi there, stranger! I'm glad you can help us with our messy code. | ||
|
|
||
|
|
||
| ## Step 1: Code Review | ||
|
|
||
| Create empty repository and make Pull Request with the whole codebase you've got from us. Then make a code review | ||
| for it. Don't hesitate to point out each concern - we want to make the code eventually perfect! | ||
| We'll be grateful for all your thoughts on it. | ||
|
|
||
|
|
||
| ## Step 2: Bug hunt | ||
|
|
||
| 1. For some reason the app doesn't fully load data. This happens on all major browsers: Chrome, Firefox, IE11. | ||
| When you resolve the issue, make sure it works on all mentioned browsers | ||
|
|
||
| 2. The user image in the history block is not aligned properly. Can you do something about it? | ||
|
|
||
|
|
||
| ## Step 3: New features | ||
|
|
||
| As you could probably see, the project lacks some more or less important features. Can you help us making it done? | ||
|
|
||
|
|
||
| 1. The username field is missing validation. Prepare a simple validation for the input field: | ||
| * not-empty | ||
| * allow only characters: `a-z`, `0-9`, `-`, `_` | ||
|
|
||
| When the value is not valid, display a red border around the field. | ||
|
|
||
| 2. The History block is just a mockup. Make it dynamic. After user data has been loaded, use the below URL: | ||
| `https://api.github.com/users/{username}/events/public` | ||
| to load user's latest events. Populate the history list with events gotten from the above step. | ||
|
|
||
| For now we want only 2 event types to be included: | ||
|
|
||
| * `PullRequestEvent` - both "opened" and "closed" Pull Requests | ||
| * `PullRequestReviewCommentEvent` | ||
|
|
||
| please take into account that other events will be implemented later. | ||
|
|
||
| Github documentation may become handy: https://developer.github.com/v3/ | ||
|
|
||
|
|
||
| ``` | ||
| **Warning:** be aware that Github uses request limiting. When it occurs, try to mitigate it. Or just make | ||
| your code perfect at first time :) | ||
| ``` | ||
|
|
||
| 3. Hide fake Profile and History fields until the data is being loaded. | ||
| We've prepare a spinner element (in HTML) to indicate loading data, but forgot to use it, Can you make it visible | ||
| when there are pending requests? | ||
|
|
||
| 4. Currently the Profile column has width of 1/3 screen. Let's change it's size: | ||
| * below 768px: make it 100% (as it is now) | ||
| * 768-1280px: make it 50% | ||
| * 1280px and above: make it 30% | ||
|
|
||
| 5. The production build is quite big: **556kB**. Can you optimize it to fit in less than 350kB? The less | ||
| the better. | ||
|
|
||
|
|
||
| # Rules | ||
|
|
||
| **Each Step should be in separate commit.** | ||
| That way you will first create a PR and then have at least 8 more commits: | ||
| * 1 containing whole code for Pull request | ||
| * 2 for bug fixes | ||
| * 5 for new features | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| { | ||
| "name": "calc", | ||
| "version": "0.1.0", | ||
| "description": "", | ||
| "main": "index.js", | ||
| "scripts": { | ||
| "start": "./node_modules/.bin/webpack-dev-server", | ||
| "build": "./node_modules/.bin/webpack -p", | ||
| "watch": "./node_modules/.bin/webpack --watch" | ||
| }, | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "ISC", | ||
| "devDependencies": { | ||
| "babel-core": "~6.26.0", | ||
| "babel-loader": "~7.1.2", | ||
| "babel-preset-env": "~1.6.0", | ||
| "clean-webpack-plugin": "~0.1.17", | ||
| "css-loader": "~0.28.7", | ||
| "extract-text-webpack-plugin": "~3.0.0", | ||
| "file-loader": "~1.1.4", | ||
| "html-loader": "~0.5.1", | ||
| "html-webpack-plugin": "~2.30.1", | ||
| "node-sass": "~4.5.3", | ||
| "sass-loader": "~6.0.6", | ||
| "style-loader": "~0.18.2", | ||
| "webpack": "~3.6.0", | ||
| "webpack-dev-server": "~2.9.1" | ||
| }, | ||
| "dependencies": { | ||
| "bulma": "^0.7.1", | ||
| "bulma-start": "^0.0.2", | ||
| "bulma-timeline": "^2.0.3", | ||
| "cash-dom": "^2.1.7" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import './assets/scss/app.scss'; | ||
| import $ from 'cash-dom'; | ||
|
|
||
|
|
||
| export class App { | ||
| initializeApp() { | ||
| let self = this; | ||
|
|
||
| $('.load-username').on('click', function (e) { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. e variable is not used in further implementation, should be ommited |
||
| let userName = $('.username.input').val(); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Username is property of App, so it would be nice to set this property by Object library. |
||
|
|
||
| fetch('https://api.github.com/users/' + userName) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no validation for userName, it could deliver some tricky behaviour, try accessing username by property, as mentioned before |
||
| .then((response)=> {response.json}) | ||
| .then(function (body) { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if response.code != 200? |
||
| self.profile = body; | ||
| self.update_profile(); | ||
| }) | ||
|
|
||
| }) | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Newline |
||
| } | ||
|
|
||
| update_profile() { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. camelCase instead of snake_case. Be consistent with your code
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. try to avoid |
||
| $('#profile-name').text($('.username.input').val()) | ||
| $('#profile-image').attr('src', this.profile.avatar_url) | ||
| $('#profile-url').attr('href', this.profile.html_url).text(this.profile.login) | ||
| $('#profile-bio').text(this.profile.bio || '(no information)') | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| @import "~bulma"; | ||
| @import "~bulma-timeline"; | ||
|
|
||
| .navbar { | ||
| border-bottom: 1px solid #eee; | ||
| margin-bottom: 8px; | ||
| } | ||
|
|
||
| .gh-username img{ | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expected space after img |
||
| width: 32px; | ||
| } | ||
|
|
||
| #profile-bio{ | ||
| margin-top:10px; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expected space after
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expected space after |
||
| } | ||
|
|
||
| .timeline .timeline-item { | ||
| padding-bottom:1em; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expected space after |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| <html lang="pl"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <title>Github-browser</title> | ||
| </head> | ||
| <body> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| <nav class="navbar"> | ||
| <div class="navbar-brand"> | ||
| <div class="navbar-item"> | ||
| <div class="field has-addons"> | ||
| <div class="control"> | ||
| <input class="input username" type="text" placeholder="enter github username"> | ||
| </div> | ||
| <div class="control"> | ||
| <button class="button is-info load-username"> | ||
| Load | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="navbar-item"> | ||
| <div class="loader is-hidden" id="spinner"></div> | ||
| </div> | ||
|
|
||
| </div> | ||
| </nav> | ||
|
|
||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="container"> | ||
| <div class="columns is-1-tablet"> | ||
| <div class="profile-container column is-one-third"> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <h2 class="subtitle is-4">Profile</h2> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="profile"> | ||
| <div class="media"> | ||
| <div class="media-left"> | ||
| <figure class="media-left image is-64x64"> | ||
| <img src="http://placekitten.com/200/200" id="profile-image"> | ||
| </figure> | ||
| </div> | ||
| <div class="media-content"> | ||
| <p class="title is-5" id="profile-name">John Smith</p> | ||
| <p class="subtitle is-6"><a href="#" id="profile-url">@johnsmith</a></p> | ||
| </div> | ||
| </div> | ||
|
|
||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="content" id="profile-bio"> | ||
| <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin ornare magna eros, eu pellentesque tortor | ||
| vestibulum ut.</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="events-container column"> | ||
| <h2 class="subtitle is-4">History</h2> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="timeline" id="user-timeline"> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="timeline-item"> | ||
| <div class="timeline-marker"></div> | ||
| <div class="timeline-content"> | ||
| <p class="heading">Jan 21, 2016</p> | ||
| <div class="content"> | ||
| <span class="gh-username"> | ||
| <img src="https://avatars.githubusercontent.com/u/2310778?"/> | ||
| <a href="https://github.com/sethii">sethii</a> | ||
| </span> | ||
| closed | ||
| <a href="https://github.com/TheSoftwareHouse/Kakunin/pull/41">pull request</a> | ||
| <p class="repo-name"> | ||
| <a href="https://github.com/TheSoftwareHouse/Kakunin">TheSoftwareHouse/Kakunin</a> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="timeline-item is-primary"> | ||
| <div class="timeline-marker is-primary"></div> | ||
| <div class="timeline-content"> | ||
| <p class="heading">Jan 19, 2016</p> | ||
| <div class="content"> | ||
| <span class="gh-username"> | ||
| <img src="https://avatars.githubusercontent.com/u/1208786?"/> | ||
| <a href="https://github.com/simonsipl">gajdus</a> | ||
| </span> | ||
| created | ||
| <a href="https://github.com/TheSoftwareHouse/Kakunin/pull/41#discussion_r176273897">comment</a> | ||
| to | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is wise to have all html as a child node. In this case would do no harm if you could tag |
||
| <a href="https://api.github.com/repos/TheSoftwareHouse/Kakunin/pulls/comments/176273897">pull request</a> | ||
| <p class="repo-name"> | ||
| <a href="https://github.com/TheSoftwareHouse/Kakunin">TheSoftwareHouse/Kakunin</a> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| <div class="timeline-item"> | ||
| <div class="timeline-marker"></div> | ||
| <div class="timeline-content"> | ||
| <p class="heading">Jan 19, 2016</p> | ||
| <div class="content"> | ||
| <span class="gh-username"> | ||
| <img src="https://avatars.githubusercontent.com/u/2310778"/> | ||
| <a href="https://github.com/aherok">sethii</a> | ||
| </span> | ||
| opened | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as mentioned in line 92 |
||
| <a href="https://github.com/TheSoftwareHouse/Kakunin/pull/41">pull request</a> | ||
| <p class="repo-name"> | ||
| <a href="https://github.com/TheSoftwareHouse/Kakunin">TheSoftwareHouse/Kakunin</a> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
|
|
||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nl |
||
| </body> | ||
| </html> | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is nice to have good intendation between HTML tags, when nesting like in XML nesting, it is easier to read which is nested where. I know that body && head && html tags are obvious, but.. nice to have it nested properly |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import styles from './assets/scss/app.scss'; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in webpack config you deliver |
||
| import {App} from "./app"; | ||
|
|
||
| const app = new App(); | ||
|
|
||
| app.initializeApp(); | ||
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.
Two newlines, expected 1