Skip to content

Comments

first commit of header seeking feedback on what to implement#99

Open
lizzkats wants to merge 3 commits intomasterfrom
createHeader
Open

first commit of header seeking feedback on what to implement#99
lizzkats wants to merge 3 commits intomasterfrom
createHeader

Conversation

@lizzkats
Copy link
Collaborator

@lizzkats lizzkats commented Mar 8, 2017

Fixes #78

Overview

I rendered an already created Navbar that I found in the project and added a placeholder for text and created a component and styling for where a user image should go. Then, with the second commit, I rendered an image from gravatar just by using a function called md5 that creates a hash based on an email. Currently, we are not making a request to Github's API. I am going to attempt it but I wanted to commit just the gravatar image for now in case I am not successful.

Data Model / DB Schema Changes

N/A

Environment / Configuration Changes

Downloaded NPM module for md5 function.

Notes

I would really appreciate more descriptions on the issues. I am trying to work on the skill of asking for really clear instructions and it has been difficult for me to just remember conversations that we had on information on the specs. I think it would better serve the learner to be able to complete really clear specs and be able to ask for clarification from at least some kind of description of the issue.

Copy link

@jaredatron jaredatron left a comment

Choose a reason for hiding this comment

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

I'm not sure I gave you the feedback you were after. Let me know if you need more guidance.

@@ -0,0 +1,11 @@
import React, { Component } from 'react'
import styles from './userimage.css'

Choose a reason for hiding this comment

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

where are you using the styles variable? If not just do import './userimage.css'

render() {
return (
<div>
<img className='user-image' src='http://placehold.it/50x50&text=slide1' />

Choose a reason for hiding this comment

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

formatting: indent this line one more

Choose a reason for hiding this comment

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

Are you use you need this div? If you do consider moving the className='user-image' to the div.

@@ -0,0 +1,4 @@
.user-image {

Choose a reason for hiding this comment

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

if you move the classname as I suggested above you could change this to .user-image > img {

@@ -1,4 +1,6 @@
.Navbar {
font-family: inherit;

Choose a reason for hiding this comment

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

these styles are indented 4 instead of 2 spaces.

@@ -1,10 +1,12 @@
import React, { Component } from 'react'
import styles from './index.css'
import UserImage from '../../atoms/Userimage/userimage'

Choose a reason for hiding this comment

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

formatting. remove this leading space

Choose a reason for hiding this comment

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

if you rename atoms/Userimage/userimage.js to atoms/Userimage/index.js you can change this import statement to import UserImage from '../../atoms/Userimage' which looks much cleaner

return (<div className={styles.Navbar}>
This is the navbar
</div>)
return <div className={styles.Navbar}>

Choose a reason for hiding this comment

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

I hate this crazy way of indenting things. Just do this

return <div className={styles.Navbar}>
  Newbie
  <UserImage />
</div>

console.log('state:', this.state)
return (
<Layout>
<Navbar />

Choose a reason for hiding this comment

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

this should be intended two move spaces

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.

2 participants