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

Stage 1 - Shortest path to success #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Stage 1 - Shortest path to success #2

wants to merge 1 commit into from

Conversation

and-ca
Copy link

@and-ca and-ca commented Mar 14, 2020

No description provided.


let resume ={
"basics": {
"name": "Richard Hendriks",

Choose a reason for hiding this comment

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

Nice Silicon Valley reference!

let element
let className

var basics = document.getElementById("basics")

Choose a reason for hiding this comment

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

The mix of ES6 and ES5 syntax is a bit confusing. Sticking to one or the other would be good for consistency :)

basics.appendChild(element)
}
var work = document.getElementById("work")
resume.work.forEach (elementArr => {

Choose a reason for hiding this comment

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

Same comment here about mixing ES5 and ES6 syntax being confusing. I would recommend using either function declarations or function expressions for consistency.

element.classList.add( className )
projects.appendChild(element)
}
})

Choose a reason for hiding this comment

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

There is a lot of repetition in your code. You could make this into one reusable function:

function displayData (property) {
       var projects = document.getElementById(property)
       resume[property]forEach (elementArr => {
        for (const [key, value] of Object.entries(elementArr)) {
            className = property + key
            element = document.createElement("P")
            element.textContent = value
            element.classList.add( className )
            projects.appendChild(element)
          }
      })
}

Then call it for work through to projects.
e.g. displayData("work");


</head>

<body onLoad="getResume()">

Choose a reason for hiding this comment

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

I would recommend adding more semantic structure to the body (e.g. header, main, section, footer), but this definitely works for the purpose of the project :)

@@ -0,0 +1,245 @@

let resume ={
Copy link
Contributor

Choose a reason for hiding this comment

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

let should be used only if the variable is not reassigned.
const is more appropriate here.

}



Copy link
Contributor

Choose a reason for hiding this comment

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

a single space is fine :-)

element.classList.add( className )
basics.appendChild(element)
}
var work = document.getElementById("work")
Copy link
Contributor

Choose a reason for hiding this comment

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

use let and const instead of var

element.classList.add( className )
volunteer.appendChild(element)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

These iterations can be cleaned up a lot by using Handlebars, Mustache, or some other templating library 😄

element.textContent = value
element.classList.add( className )
publications.appendChild(element)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This works wel, but you're setting classnames, textContent, etc. over and over again in all these blocks. can you think of a way to reduce repetition?

</div>
<div id="projects">
<h2>Projects</h2>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This, too, can be reduced by using templating engines

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.

3 participants