-
-
Notifications
You must be signed in to change notification settings - Fork 454
Bashir week 5 #1012
base: master
Are you sure you want to change the base?
Bashir week 5 #1012
Conversation
to follow previously sent common practice and instructions fixed some typos
Minor changes to make language consistent and simplify it where needed.
| //Write your code in here | ||
| arrayOfPeople.forEach(function (person) { | ||
| let h1 = document.createElement("h1"); | ||
| let h2 = document.createElement("h2"); |
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.
Hi Bashir - this is looking good at the moment.
| arrayOfPeople.forEach(function (person) { | ||
| let h1 = document.createElement("h1"); | ||
| let h2 = document.createElement("h2"); | ||
| content.appendChild(h1).textContent = person.name; |
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 would advise putting stuff like this onto separate lines for clarity.
| content.appendChild(h1).textContent = person.name; | |
| h1.textContent = person.name; | |
| content.appendChild(h1); |
| */ | ||
| function insertShoppingList(shoppingList) { | ||
| //Write your code in here | ||
| var unorderedList = document.createElement("ul"); |
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.
Try to avoid var keyword if you can - stick to using const or let
| //Write your code in here | ||
| var unorderedList = document.createElement("ul"); | ||
| shoppingList.forEach((item) => { | ||
| var listItem = document.createElement("li"); |
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.
listItem is an example of a good variable name - much clearer to read
| function insertBooks(books) { | ||
| //Write your code in here | ||
| let bookslist = document.createElement("ul"); | ||
| let imageList = ["./img/1.jpg", "./img/2.jpg", "./img/3.jpg"]; |
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.
These images won't display anything - you might need to find some images off Google images
| let bookName = document.createElement("p"); | ||
| bookName.innerText = `${item.title}-${item.author}`; | ||
| let bookImg = document.createElement("img"); | ||
| bookImg.src = imageList[i]; |
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.
At the moment there is a ReferenceError appearing in the Chrome console. It is stating that i is not defined
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.
Can you try and work out where this error is coming from ?
| let blueButton = document.querySelector("#blueBtn"); | ||
| let jumbotron = document.querySelector(".jumbotron"); | ||
| let donAndVolBtns = document.querySelectorAll(".btn-lrg"); | ||
| blueButton.addEventListener("click", function () { |
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.
Some good variable names here
| isValid = false; | ||
| } | ||
| if (emailInput.value.indexOf("@") === -1 || emailInput.value.length < 1) { | ||
| emailInput.style.backgroundColor = "red"; |
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.
Setting the whole background colour is a bit dramatic for the user. Think of a more subtle way in which you can inform the user that there is an issue with the page.
|
|
||
| function areValidElements() { | ||
| let isValid = true; | ||
| if (nameInput.value.length < 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.
Could also check here that nameInput.value.length === 0 too
|
|
||
| /* Submit button event Listener function*/ | ||
|
|
||
| function submitButtonFunction() { |
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 like how you have put this logic in a separate place - inside the submitButtonFunction
Your Details
Your Name:
Your City:
Your Slack Name:
Homework Details
Module:
Week: