Nikita-w1-Browsers #31

NStefanchuk · 2025-03-18T12:16:21Z

No description provided.

RHSebregts

I think you did pretty well on the assignment. The cat is moving, but could still use some love.
If I'm being completely honest, the clock/date function being in British time has a bit of an AI smell to it. If my hunch there is correct, that is fine, but I think it'd be good to properly check and understand each line when you do use it.

RHSebregts · 2025-03-25T09:26:11Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

+  const listBooks = document.createElement('ul');
+  listBooks.classList.add('list__books');
+
+  for (let i = 0; i < books.length; i++) {


I like the idea of a loop here, but I think we could also use a different kind of loop to go through all the elements in the array.

RHSebregts · 2025-03-25T09:27:41Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

+    itemBook.classList.add('item__book');
+
+    if (books[i].alreadyRead) {
+      itemBook.classList.add('item__book-read');


I'm personally not a big fan of this casing. I would love to see a more conventional one, like kebab, camel, or snake.

Ok, ignore that, apparently this is very common for BEM, which I'm not super familiar with. So I guess I stand corrected!

okey, thanks! I use it a lot
But I will try kebab later

RHSebregts · 2025-03-25T09:28:38Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

+    const itemBook = document.createElement('li');
+    itemBook.classList.add('item__book');
+
+    if (books[i].alreadyRead) {


I think this could be a great place to use a ternary operator on the DOM element.

RHSebregts · 2025-03-25T09:30:04Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

+    imgBook.src = books[i].bookUrl;
+    imgBook.alt = `Cover of ${books[i].title} by ${books[i].author}`;
+
+    paragraph.textContent = books[i].title + ' ' + books[i].author;


You are using both ways to create a string here, I think it could be nicer (and less error-prone) if you'd use the same way for both.

RHSebregts · 2025-03-25T09:33:05Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

@@ -28,18 +56,21 @@ function main() {
      author: 'Don Norman',
      isbn: '978-0465050659',
      alreadyRead: false,
+      bookUrl: 'assets/the_design_of_everyday_things.jpg',


Good idea to add the url for the book image! The naming of the key is somewhat misleading though, since this is not a url to the book, but to the image of the book. Could you think of a better name?
It is also not necessary to use book in the key, since we already know it's a book object. That's why we also don't see bookAuthor or bookIsbn.

RHSebregts · 2025-03-25T09:43:52Z

2-Browsers/Week1/assignment/ex4-whatsTheTime/index.js

 }

-// TODO execute `addCurrentTime` when the browser has completed loading the page
+window.addEventListener('DOMContentLoaded', () => {


Why did you choose this specific event to listen for?

We need to be sure that our DOM tree is fully completed (ready). Or I need to change it?

2-Browsers/Week1/assignment/ex5-catWalk/index.js

RHSebregts · 2025-03-25T09:48:35Z

2-Browsers/Week1/assignment/ex5-catWalk/index.js

+    isCatCenter = false;
+  }
+
+  if (startPoint >= clientWidth / 2 - widthCat / 2 && !isCatCenter) {


I have some difficulty with reading this if condition. A lot is happening, and I'm also worried that the order of operations might get messed up with the double division and subtraction. Could you try to rewrite this so that it's a bit easier to read and safer to execute?

RHSebregts · 2025-03-25T09:50:06Z

2-Browsers/Week1/assignment/ex5-catWalk/index.js

+  if (startPoint >= clientWidth / 2 - widthCat / 2 && !isCatCenter) {
+    clearInterval(timerId);
+    movingCat.src =
+      'https://media1.tenor.com/images/2de63e950fb254920054f9bd081e8157/tenor.gif';


It would be nice if the cat walking and dancing sources are a well named const, so that the reader of the code doesn't have to read the URL, but can read "amazingDancingCatGIF" or "slowlyWalkingCatPicture" and know what's being shown in what situation.

RHSebregts · 2025-03-25T09:50:35Z

2-Browsers/Week1/assignment/ex5-catWalk/index.js

 }

-// TODO execute `catWalk` when the browser has completed loading the page
+window.addEventListener('DOMContentLoaded', () => {


Why did you choose this specific event to listen for?

We need to be sure that our DOM tree is fully completed (ready)

RHSebregts

Great job, LGTM!

RHSebregts · 2025-04-05T10:19:57Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

-    paragraph.textContent = books[i].title + ' ' + books[i].author;
+    imgBook.src = book.image;
+    imgBook.alt = `Cover of ${book.title} by ${book.author}`;
+    // paragraph.textContent = book.title + ' ' + book.author;


You could still delete this comment, but don't worry about it

okey, I just left it to remember the different way

RHSebregts · 2025-04-05T10:22:13Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

@@ -56,21 +59,21 @@ function main() {
      author: 'Don Norman',
      isbn: '978-0465050659',
      alreadyRead: false,
-      bookUrl: 'assets/the_design_of_everyday_things.jpg',
+      image: 'assets/the_design_of_everyday_things.jpg',


If I were in a particularly bad mood I would say, "this is not actually the image, but a link to the image, so use imageLink or imageURL", but since it's sunny out and the skies are blue, so I think it's ok!

I was in danger, but it seems we all got lucky with the weather in the Netherlands this spring

RHSebregts · 2025-04-05T10:22:50Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

@@ -21,30 +21,33 @@ function createBookList(books) {
  const listBooks = document.createElement('ul');
  listBooks.classList.add('list__books');

-  for (let i = 0; i < books.length; i++) {
+  books.forEach((book) => {


RHSebregts · 2025-04-05T10:31:10Z

2-Browsers/Week1/assignment/ex1-bookList/index.js

-    imgBook.alt = `Cover of ${books[i].title} by ${books[i].author}`;
-
-    paragraph.textContent = books[i].title + ' ' + books[i].author;
+    imgBook.src = book.image;


Don't you just love how much cleaner using book is opposed to books[I]? Also, today I learned: forEach is hella slow, and for ... of is way more performant?
Anyway, code wise, this is all good!

Oh, thank you. Interesting graph and site

RHSebregts · 2025-04-05T10:35:13Z

2-Browsers/Week1/assignment/ex5-catWalk/index.js

Great, the cats walks well, and the code looks a lot cleaner and more readable!

thanks, but I think the cat's starting to make me crazy. I found it again in the API module

NStefanchuk · 2025-04-07T18:51:13Z

Great job, LGTM!

https://www.reddit.com/r/ProgrammerHumor/comments/w92k2i/lgtm/
Now I know what LGTM is :)
Thank you for reviewing my code

NStefanchuk added 2 commits March 18, 2025 13:14

1 week browsers

e47e9aa

tests added

edf7328

RHSebregts self-requested a review March 25, 2025 08:02

RHSebregts self-assigned this Mar 25, 2025

RHSebregts added the Review in progress label Mar 25, 2025

RHSebregts requested changes Mar 25, 2025

View reviewed changes

RHSebregts added Needs work and removed Review in progress labels Mar 25, 2025

Mentor's comments fixed

ba4e0fa

RHSebregts approved these changes Apr 5, 2025

View reviewed changes

RHSebregts added Approved and removed Needs work labels Apr 5, 2025

Nikita-w1-Browsers #31

Are you sure you want to change the base?

Nikita-w1-Browsers #31

Uh oh!

Conversation

NStefanchuk commented Mar 18, 2025

Uh oh!

RHSebregts left a comment

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!

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!

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!

RHSebregts left a comment

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!

Choose a reason for hiding this comment

Uh oh!

NStefanchuk commented Apr 7, 2025

Uh oh!

Uh oh!