Document Object Model - practice #159

zhenyakornilov · 2022-08-15T11:59:47Z

Document Object Model - practice

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

zhenyakornilov · 2022-08-16T20:05:51Z

Removed DOM calls from loop in renderButtons function

zhenyakornilov · 2022-08-19T17:40:11Z

Could you give some advice how to load image faster?

kasionio

Nice job, looking good! Please take a look at the comments. And I found some weird behavior. A highlighted zone is also active but doesn't have a hover effect

kasionio · 2022-08-22T20:05:47Z

submissions/zhenyakornilov/js-dom/css/styles.css

+  box-sizing: border-box;
+}
+
+ul,


this style applies just for li tags, so you can leave just one tag here
https://developer.mozilla.org/en-US/docs/Web/CSS/list-style

kasionio · 2022-08-22T20:09:22Z

submissions/zhenyakornilov/js-dom/css/styles.css

+  background: none;
+  color: inherit;
+  border: none;
+  padding: 0;


you specified this property for all elements in the first lines

kasionio · 2022-08-22T20:19:11Z

submissions/zhenyakornilov/js-dom/index.html

@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html lang="en" id="root">


It is a common practice to divide responsibility - ids for JS, classes for styling. Better to use the wrapper in this case and set styles for wrapper, not for htmltag. Using html styling is even bad for performance reasons

can I use :root pseudo-class in CSS instead?

the common use of :root is for variables declaration. For such files root will be always html
https://developer.mozilla.org/en-US/docs/Web/CSS/:root

kasionio · 2022-08-22T20:23:51Z

submissions/zhenyakornilov/js-dom/css/styles.css

+.nav-item {
+  border-bottom: 1px solid #f5f5dc;
+  width: 20rem;
+  margin: 10;


what are the units?

kasionio · 2022-08-22T20:28:05Z

submissions/zhenyakornilov/js-dom/index.html

+    <header class="header">
+      <span class="dropdown-wrap">
+        <button class="dropdown">
+          <i class="fa-solid fa-bars fa-lg"></i>


fa-solid fa-bars fa-lg. What are these classes for?

It's Font Awesome prebuilt classes, I thought it is not needed to change them.

you are right. my bad

kasionio · 2022-08-22T20:38:38Z

submissions/zhenyakornilov/js-dom/js/index.js

+}
+
+function renderArticle(museumObj, content) {
+  const contentHTML = `


You can use object destructuring
https://javascript.info/destructuring-assignment#object-destructuring

kasionio · 2022-08-22T20:42:09Z

submissions/zhenyakornilov/js-dom/js/index.js

+}
+
+function toggleSelectedButton(museums, content) {
+  let selectedButton = document.getElementsByClassName("nav-item")[1];


Logically, default value is the first element of the array, not second

kasionio · 2022-08-22T20:44:14Z

submissions/zhenyakornilov/js-dom/js/utils.js

+function changeTheme() {
+
+  const checkbox = document.getElementById("switcher");
+  checkbox.addEventListener("change", () => {


better to separate the handler function from a listener.
el.addEventListener("change", handler);
function handler {}

kasionio · 2022-08-22T20:49:05Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+document.addEventListener("DOMContentLoaded", main);
+
+function renderButtons(arr, menu) {


arr sounds too generic. We can name it listOfCards for example or what you prefer more

kasionio · 2022-08-22T20:49:50Z

submissions/zhenyakornilov/js-dom/js/index.js

+function renderButtons(arr, menu) {
+  let buttonsHTML = ``;
+
+  arr.forEach(({ btnName }) => {


map looks like the optimal choice

kasionio · 2022-08-22T21:08:57Z

About images - the simplest way is to compress images or use a lightweight format such as webp. One more option - while an image is loading you can set some default placeholder instead. Also, HTML has a cool tag picture, take a look on it
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture
https://developers.google.com/speed/webp

zhenyakornilov · 2022-08-24T17:52:42Z

Tried to make all fixes accordingly to remarks.

zhenyakornilov · 2022-08-24T17:56:12Z

I have questions about some remarks, see in comments above.

zhenyakornilov · 2022-08-24T17:59:11Z

Nice job, looking good! Please take a look at the comments. And I found some weird behavior. A highlighted zone is also active but doesn't have a hover effect

this one looks to be fixed too

kasionio

Great progress, but let's improve JS part 🚀

kasionio · 2022-08-25T15:15:47Z

submissions/zhenyakornilov/js-dom/index.html

+    </main>
+    <footer class="footer">
+      <a class="footer-link" href="https://github.com/zhenyakornilov" target="_blank">
+        <span><i class="fa-brands fa-github"></i> Yevhen Kornilov</span>


it is a good practice to place one tag in a line

kasionio · 2022-08-25T15:21:33Z

submissions/zhenyakornilov/js-dom/css/styles.css

+    grid-template-areas: "header-top header-top" "content-main content-main" "footer-bottom footer-bottom";
+  }
+
+  .dropdown {


Sorry, I don't understand this remark here.

Do You mean something wrong with grid-template-areas?

You have two similar classes

Yet I don't get what I have to change here(

look at line 132

I don't understand what's wrong here. I defined .dropdown in CSS with display none to hide and then changed this in media query to make it visible.

kasionio · 2022-08-25T15:22:24Z

submissions/zhenyakornilov/js-dom/css/styles.css

+}
+
+.dropdown-wrap {
+  display: inline;


isn't it the default display value?

kasionio · 2022-08-25T15:23:08Z

submissions/zhenyakornilov/js-dom/css/styles.css

+  outline: inherit;
+}
+
+:root {


you can use html if you want

I change html font-size since I use rem and when html font-size is equal to 62.5% then 1 rem is like 10 pixels, 2.1rem is 21 pixels etc. It makes more clear to me rem values to interpret them as pixels.

kasionio · 2022-08-25T15:23:50Z

submissions/zhenyakornilov/js-dom/css/styles.css

+}
+
+.header .check-switch:checked + .theme-switcher .circle {
+  transform: translatex(2.4rem);


X from capital :))

too specific selector, WDYT?

kasionio · 2022-08-25T15:50:30Z

submissions/zhenyakornilov/js-dom/js/index.js

@@ -0,0 +1,89 @@
+"use strict";


To make JS more readable let's try to do some improvements:

we can declare all elements which we found by getElement at the top of the file;
then we can declare all functions. Functions should be pure, which means do one thing;
at the bottom, we can add listeners;

Changed index.js. Did You mean something like it looks now?

Not yet. Divide listeners and functions
element.addEventListener(event, listener);
Look below, the listener is bold

function showMuseumInfoByClick(museums, content, menu) { menu.addEventListener("click",function ({ target }) { if (target.tagName.toLowerCase() === "button") { const currentContentObject = museums.filter( ({ btnName }) => btnName === target.textContent ); renderArticle(currentContentObject[0], content); } }); }

separated listeners and functions

kasionio · 2022-08-25T15:53:57Z

submissions/zhenyakornilov/js-dom/js/utils.js

@@ -0,0 +1,15 @@
+function toggleDarkClass() {
+


no need to add spaces inside a function. It should looks like a block of code

kasionio · 2022-08-25T15:56:37Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+  const buttons = document.getElementsByClassName("nav-item");
+
+  function getContextObj() {


please explain what function name getContextObj mean. I really want to understand :)

This function returns object from museums array to render as content.

I mean name. getConteNtObject?

kasionio · 2022-08-25T16:07:30Z

submissions/zhenyakornilov/js-dom/js/index.js

+  renderArticle(getContextObj(), content);
+
+  for (let button of buttons) {
+    button.addEventListener("click", function () {


here you added a listener to every button. why not add a listener to the navigation menu?

kasionio · 2022-08-25T16:11:31Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+  renderArticle(getContextObj(), content);
+
+  for (let button of buttons) {


try to use a different approach. for...of is not needed here. now it seems that you take different parts of code from different sources

zhenyakornilov · 2022-08-26T17:57:54Z

Tried to make changes accordingly to recommendations, but still have some questions above.

kasionio · 2022-08-29T11:52:04Z

Please do not resolve comments, I need to see them

kasionio

Nice job! Please take a look at the comments and make corresponding changes. Also add hover effect to theme-switcher

kasionio · 2022-08-29T11:56:36Z

submissions/zhenyakornilov/js-dom/css/styles.css

+    grid-template-areas: "header-top header-top" "content-main content-main" "footer-bottom footer-bottom";
+  }
+
+  .dropdown {


look at line 132

kasionio · 2022-08-29T11:57:46Z

submissions/zhenyakornilov/js-dom/index.html

+<body class="body">
+  <div class="wrapper">
+    <header class="header">
+      <span class="dropdown-wrap">


You deleted this class in the CSS file

kasionio · 2022-08-29T11:59:40Z

submissions/zhenyakornilov/js-dom/js/index.js

+    <p class="museum-desc">${description}</p>
+  </div>
+  <div class="additional-wrapper">
+    <p class="museum-link">


Please take a look at when you can use p tag and find appropriate tag
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/p

kasionio · 2022-08-29T12:03:08Z

submissions/zhenyakornilov/js-dom/index.html

+      <ul class="menu-items" id="menu"></ul>
+    </nav>
+    <main class="content">
+      <article class="content-body" id="content"></article>


article should have a header
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article

I have misunderstanding here. I have h2 tag added with JS inside article tag, isn't it can be counted as header? Or shall I set h2 tag inside index.html? May I just change article tag to div then?

you are right, I missed h2. All is fine here

kasionio · 2022-08-29T12:05:28Z

submissions/zhenyakornilov/js-dom/js/index.js

+import switchTheme from "./utils.js";
+
+function main() {
+  const dropdownBtn = document.getElementById("dropdown");


you get element by id dropdown and name it dropdownBtn, which means button. seems inconsistent naming here

kasionio · 2022-08-29T12:11:02Z

submissions/zhenyakornilov/js-dom/js/index.js

@@ -0,0 +1,89 @@
+"use strict";


Not yet. Divide listeners and functions
element.addEventListener(event, listener);
Look below, the listener is bold

function showMuseumInfoByClick(museums, content, menu) { menu.addEventListener("click",function ({ target }) { if (target.tagName.toLowerCase() === "button") { const currentContentObject = museums.filter( ({ btnName }) => btnName === target.textContent ); renderArticle(currentContentObject[0], content); } }); }

kasionio · 2022-08-29T12:15:49Z

submissions/zhenyakornilov/js-dom/js/index.js

+  </div>
+  <div class="additional-wrapper">
+    <p class="museum-link">
+      <span>Visit </span>


why span? you can leave this text without wrapping in a tag

kasionio · 2022-08-29T12:17:48Z

submissions/zhenyakornilov/js-dom/js/index.js

+  const contentHTML = `
+  <h2 class="museum-title">${name}</h2>
+  <div class="desc-wrapper">
+    <img class="museum-image" src="${imageSrc}" alt="photo of museum">


alt should be moved to data.js as one of the paramters, because it can't be the same for different images

zhenyakornilov · 2022-08-31T17:45:53Z

New chnages added, but some questions left.

kasionio

Much better now, great job!
Some small changes left. And about the hover effect on switchTheme button. All interactive elements should have pointer cursor on hover

kasionio · 2022-08-31T19:44:14Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+function showMuseumInfo({ target }) {
+  if (target.tagName.toLowerCase() === "button") {
+    const currentContentObject = museums.filter(


you can try to use spread here
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

kasionio · 2022-08-31T19:47:53Z

submissions/zhenyakornilov/js-dom/js/index.js

+}
+
+function showMuseumInfo({ target }) {
+  if (target.tagName.toLowerCase() === "button") {


why do not use .closest here?

also better to prevent rendering when you click on the same button which already selected

kasionio · 2022-08-31T19:53:46Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+  themeSwitcher.addEventListener("change", toggleDarkClass);
+  dropdownBtn.addEventListener("click", openDropdownMenu);
+  listMenu.addEventListener("click", showMuseumInfo);


good article to understand the events bubbling process
https://javascript.info/bubbling-and-capturing

zhenyakornilov · 2022-09-01T17:54:53Z

Added latest changes

kasionio

Let's make last small improvements ;)

kasionio · 2022-09-04T18:25:17Z

submissions/zhenyakornilov/js-dom/js/index.js

+const contentBody = document.getElementById("content");
+
+function renderButtons(museumsObj, menu) {
+  let buttonsHTML = museumsObj


if you have plural, better name it like buttonsListHtml

kasionio · 2022-09-04T18:28:21Z

submissions/zhenyakornilov/js-dom/js/index.js

+
+function showMuseumInfo({ target }) {
+  if (target.closest(".btn-text")) {
+    let currentSelectedBtn = target.closest(".nav-item");


you can move it higher and then make instead of two one if statement

kasionio · 2022-09-04T18:37:36Z

submissions/zhenyakornilov/js-dom/js/index.js

+    let currentSelectedBtn = target.closest(".nav-item");
+    if (!currentSelectedBtn.classList.contains("active")) {
+      const [currentContentObject] = museums.filter(
+        ({ btnName }) => btnName === target.textContent


Better use id to identify the target item, not the text. For example, we can have two items with the same button name. So, in such cases we should always use id for DOM manipulation

zhenyakornilov · 2022-09-05T12:11:56Z

Added new changes accordingly to remarks.

kasionio

Fair enough, I guess :) Good job 👍

A-Ostrovnyy added the task-DOM label Aug 15, 2022

kasionio self-requested a review August 22, 2022 20:04

kasionio self-assigned this Aug 22, 2022

kasionio requested changes Aug 22, 2022

View reviewed changes

zhenyakornilov added 8 commits August 23, 2022 15:58

Add task files

bdfa72d

Formatting

278851c

DOM calls optimization

5a806c7

Fix text overlap on resize

1748e79

Some DOM calls optimizations and formatting

02f03bb

Change files, create main function

d5b73dc

Add use strict directive

d66d78b

Make some changes accordingly to remarks

b4ff9cf

zhenyakornilov force-pushed the js-dom branch from 1289e92 to b4ff9cf Compare August 23, 2022 12:59

Cover mistakes, fix bugs

ef4c363

zhenyakornilov requested a review from kasionio August 24, 2022 17:59

kasionio requested changes Aug 25, 2022

View reviewed changes

zhenyakornilov added 7 commits August 26, 2022 14:28

Add minor changes accordingly to remarks

5d71b53

Add empty line

9f22d56

Use common media queries, reduce selectors specificity

adff28e

Fix sidebar focus

3f92f01

Render content by click improvements

980055c

Remove console.log statements

e4007a9

Update styles file

ae6e626

zhenyakornilov requested a review from kasionio August 26, 2022 17:58

kasionio requested changes Aug 29, 2022

View reviewed changes

zhenyakornilov added 2 commits August 31, 2022 19:53

Make some improvements and fixes

8768ab1

Add hover effect for theme switcher

809ef44

zhenyakornilov requested a review from kasionio August 31, 2022 17:46

kasionio requested changes Aug 31, 2022

View reviewed changes

Some improvements, formatting

173631a

zhenyakornilov requested a review from kasionio September 1, 2022 17:55

kasionio requested changes Sep 4, 2022

View reviewed changes

Add improvements accordingly to remarks

6fee8bc

zhenyakornilov requested a review from kasionio September 5, 2022 12:12

kasionio approved these changes Sep 5, 2022

View reviewed changes

kasionio merged commit 3c6dc96 into kottans:main Sep 5, 2022


		document.addEventListener("DOMContentLoaded", main);

		function renderButtons(arr, menu) {


		const buttons = document.getElementsByClassName("nav-item");

		function getContextObj() {


		renderArticle(getContextObj(), content);

		for (let button of buttons) {

Document Object Model - practice #159

Document Object Model - practice #159

Conversation

zhenyakornilov commented Aug 15, 2022

Document Object Model - practice

zhenyakornilov commented Aug 16, 2022

zhenyakornilov commented Aug 19, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasionio Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyakornilov Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasionio commented Aug 22, 2022 • edited Loading

zhenyakornilov commented Aug 24, 2022

zhenyakornilov commented Aug 24, 2022

zhenyakornilov commented Aug 24, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyakornilov commented Aug 26, 2022

kasionio commented Aug 29, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyakornilov commented Aug 31, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyakornilov commented Sep 1, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasionio Aug 22, 2022 •

edited

Loading

zhenyakornilov Aug 23, 2022 •

edited

Loading

kasionio commented Aug 22, 2022 •

edited

Loading