Homework for first week js2 by Jalalalattar · Pull Request #504 · HackYourFuture/JavaScript2

Jalalalattar · 2019-12-22T00:36:24Z

Review

neveenatik · 2019-12-23T13:55:01Z

+<body>
+  <h1>My book list</h1>
+
+  <script>


In practice it is rare to have script inside of a script tag. Try to keep JavaScript external by moving the code to . a separate js file and using src attribute in script element to reference it for example

<script src='./1-books.js' type='text/javascript'></script>

The separate code makes it easier for web browsers to cache which enhances performance (here is an article about javascript performance optimisation https://www.keycdn.com/blog/javascript-performance)

neveenatik · 2019-12-23T13:57:03Z

+    title : 'Underworld',
+    author : 'Don DeLillo',
+    alreadyRead :true   
+    }


excercise requires three favourite books I see only two 😛

neveenatik · 2019-12-23T14:05:20Z

+// 5
+
+      if (obj.alreadyRead === true) {
+        p.style.color = 'green';


instead of using an inline style you can have a css file with class red/green (for example) and toggle classes here is an article that explains more about this:
https://javascript.info/styles-and-classes

neveenatik · 2019-12-23T14:05:40Z

+    for (let obj of books) {
+// 2
+      const p = document.createElement('p');
+      p.innerText = `${obj.title} was written by ${obj.author.toUpperCase()}`;


Nice use of templating 😉

neveenatik · 2019-12-23T14:08:31Z

+    }
+    // 4
+    let img1 = document.createElement('img');
+    let img2 = document.createElement('img');


why did you have img outside the for of loop. Think about how is img1 different than img2 and how can you solve that. In the end you need an image for each book right?

neveenatik · 2019-12-23T14:10:45Z

+    let img2 = document.createElement('img');
+    img1.src = 'https://upload.wikimedia.org/wikipedia/en/thumb/e/e1/ArtistOfTheFloatingWorld.jpg/220px-ArtistOfTheFloatingWorld.jpg';
+    img2.src = 'http://xoxoafterdark.com/wp-content/uploads/2014/10/Delillo-Underworld.jpg';
+    let lis = document.querySelectorAll('li');


Think about why would you query the list items you already have them inside of the for of loop right?

neveenatik · 2019-12-23T14:11:14Z

+    img1.src = 'https://upload.wikimedia.org/wikipedia/en/thumb/e/e1/ArtistOfTheFloatingWorld.jpg/220px-ArtistOfTheFloatingWorld.jpg';
+    img2.src = 'http://xoxoafterdark.com/wp-content/uploads/2014/10/Delillo-Underworld.jpg';
+    let lis = document.querySelectorAll('li');
+    lis[0].appendChild(img1).style.width = '350px';


again classes vs inline style 🙂

neveenatik · 2019-12-23T14:16:48Z

+      <li>Favorite food: <span id="fav-food"></span></li>
+      <li>Hometown: <span id="hometown"></span></li>
+    </ul>   
+	<script>


same comment as exercise one javascript and style better be in external files (unless exercise indicates to do so) 👍

neveenatik · 2019-12-23T14:22:01Z

+		document.querySelector('#hometown').innerText='London';
+
+		const items=document.querySelectorAll('ul li');
+		items.forEach(item=> {item.classList.add('list-item')})


Good work. However good to keep in mind that this way of iterating is not supported by all browsers Here is more read about iteration options on nodeList
https://css-tricks.com/snippets/javascript/loop-queryselectorall-matches/

neveenatik · 2019-12-23T14:24:36Z

+
+		const styleTag=document.createElement('style');
+		document.querySelector("head").appendChild(styleTag);
+		document.querySelector('style').innerText= '.list-item {color:red}'


I believe you misunderstood the requirement here or got carried away manipulating using javascript 😄

The idea is to add a tag in the html file and have a css file with the class list-item which sets font color to red.

neveenatik · 2019-12-23T14:25:33Z

+
+		const img = document.createElement('img');
+		img.src = 'https://images-wixmp-ed30a86b8c4ca887773594c2.wixmp.com/f/d9585883-0c30-448b-8cee-6eac6d5c695f/d80kp71-a8d13ec7-6e9a-4268-9312-222184443314.png/v1/fill/w_1024,h_576,q_75,strp/real_madrid_david_beckham_wallpaper_by_sameerhd-d80kp71.png?token=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJ1cm46YXBwOjdlMGQxODg5ODIyNjQzNzNhNWYwZDQxNWVhMGQyNmUwIiwic3ViIjoidXJuOmFwcDo3ZTBkMTg4OTgyMjY0MzczYTVmMGQ0MTVlYTBkMjZlMCIsImF1ZCI6WyJ1cm46c2VydmljZTppbWFnZS5vcGVyYXRpb25zIl0sIm9iaiI6W1t7InBhdGgiOiIvZi9kOTU4NTg4My0wYzMwLTQ0OGItOGNlZS02ZWFjNmQ1YzY5NWYvZDgwa3A3MS1hOGQxM2VjNy02ZTlhLTQyNjgtOTMxMi0yMjIxODQ0NDMzMTQucG5nIiwid2lkdGgiOiI8PTEwMjQiLCJoZWlnaHQiOiI8PTU3NiJ9XV19.DsmMSnKtljCP7zEAAUsax4S3Lf9oASSChxWHmyPAT0g'
+		document.body.appendChild(img);


Good work 👍

neveenatik · 2019-12-23T15:09:00Z

@@ -0,0 +1,7 @@
+function hijackGoogleLogo () {
+    let googleImage=document.querySelector('#hplogo')


this selector didn't work on my google version it would be interesting to see if it works on yours. can you make a screenshot of the element with this id ?

neveenatik · 2019-12-23T15:14:39Z

+
+<body>
+    <div id="klok"></div>
+    <script src="./showCurrentTime.js">


I think you renamed your javascirpt file and forgot to update the src here therefore currently code doesn't run 😨

neveenatik · 2019-12-23T15:16:01Z

+<head>
+    <title>Klok :)</title>
+    <style>
+        div {


For the same reason explained above I believe it is better to use a css file and keep style in there instead of internal style tag

neveenatik · 2019-12-23T15:16:39Z

+    const tijd = datum.toLocaleTimeString();
+    document.querySelector('#klok').innerHTML = tijd;
+}
+setInterval(eenklok, 1000);


Good job 👍

neveenatik · 2019-12-23T15:25:27Z

+        function catWalk() {
+
+            setInterval(frame, 50)
+            function frame() {


Good work however I wouldn't declare a function inside of a function simply because no one can use the inner function outside the outer function.
I think it would be better to keep all functionality inside of catWalk function and then call catWalk with setInterval every 50 ms.

function catWalk() { ... all functionality } setInterval(catWalk, 50);

neveenatik · 2019-12-23T15:27:44Z

+            setInterval(frame, 50)
+            function frame() {
+
+                left = left > 1500 ? 0 : left;


What if the screen size is less than 1500? maybe look into getting the screen width using javascript for example using window.screen.width from webAPI
https://developer.mozilla.org/en-US/docs/Web/API/Screen/width

neveenatik · 2019-12-23T15:30:51Z

+
+                    }
+                } else {
+                    img.style.left;


I'm not sure what did you want to do here it looks like you have read the img.style.left but didn't change or store it!

neveenatik

Nice work I liked the way you solved these exercises I left some comments for you please reach out through slack if any of these points aren't explained well to you 🙂
I also noticed you forgot to submit Project: Random Quote Generator

Homework for first week js2

7a4f6af

neveenatik self-assigned this Dec 23, 2019

neveenatik added the review in progress label Dec 23, 2019

neveenatik reviewed Dec 23, 2019

View reviewed changes

neveenatik added needs work reviewed and removed review in progress labels Dec 23, 2019

Jalalalattar added 3 commits January 11, 2020 15:08

There are some fixes

923604b

fixed

5075c9b

Project week1 is finish

fe77989

NoerGitKat closed this Mar 5, 2020

		@@ -0,0 +1,7 @@
		function hijackGoogleLogo () {
		let googleImage=document.querySelector('#hplogo')

Uh oh!

Conversation

Jalalalattar commented Dec 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neveenatik Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neveenatik Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neveenatik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neveenatik Dec 23, 2019 •

edited

Loading

neveenatik Dec 23, 2019 •

edited

Loading

neveenatik left a comment •

edited

Loading