My PR width DOM-api #400

Nik3264 · 2022-09-02T10:25:06Z

Document Object Model

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

github-actions · 2022-09-02T17:38:08Z

Hey!

Congratulations on your PR! 😎😎😎

Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code.

Go through the requirements/most common mistakes listed/linked below and fix the code as appropriate.

If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat.

When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly.

Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒

Please, make sure you haven't made common mistakes

Universal recommendations:

Give variables and functions meaningful names. Avoid generic names like item, element, key, object, array or their variations. Exception: helper functions that are specifically and intentionally designed to be multipurpose.
Function names should start with a verb as they denote actions; variables are normally nouns; boolean variables/functions start with is, does, has etc; variable containing multiple entities and functions returning lists contain entity name in plural form.
Have consistent code style and formatting. Employ Prettier to do all dirty work for you.
Use common sense or seek for an advice whenever requirements look ambiguous or unclear.

Also take a note of the requirements above and follow them in all your future projects.

By the way, you may proceed to the next task before this one is reviewed and merged.

Sincerely yours,
Submissions Kottachecker 😺

Nik3264 · 2022-09-02T18:05:36Z

I completed the self-test. I don't know what's wrong here. Perhaps I misunderstood the problem. I will be glad to receive recommendations on my mistakes in order to avoid them in the future. Thanks.

Nik3264 · 2022-09-04T20:54:27Z

Please, review.

lysenko-sergey-developer · 2022-09-14T20:56:54Z

@Nik3264 Good work!🥇

submissions/Nik3264/js-dom/index.js

lysenko-sergey-developer · 2022-09-14T20:58:17Z

submissions/Nik3264/js-dom/index.html

+        </nav>
+        <div class="content">
+            <div class="item">
+


A redundant empty line. Configure your code editor to see redundant space/tabs/empty lines.

Ok, I configured my code with Prettier.

Still exist) https://github.com/kottans/frontend-2022-homeworks/pull/400/files#diff-5acd093c4ee3aca60811a151969e036db3e07bb9ee1c3848f8a64b3f18aa8dceR27

lysenko-sergey-developer · 2022-09-14T21:06:51Z

UI/UX Improvements:

Work on your mobile version.
Change styles for item title header. There is too much space between header and image.
Blinking animation for content is not the best choice. Try to play with another animation or just remove it.

…space between header and img, added function render, renamed array

Nik3264 · 2022-09-15T11:02:37Z

I removed blinking animation, reduced the space between header and image with media-query. I am not sure whether it was exactly that what I had to do.
Thank you for all your comments. I tried my best to correct my mistakes ))

lysenko-sergey-developer · 2022-09-15T20:10:15Z

Good work! Let's try to put a cherry on the cake :)

lysenko-sergey-developer · 2022-09-15T19:07:49Z

submissions/Nik3264/js-dom/index.html

+        </nav>
+        <div class="content">
+            <div class="item">
+


Still exist) https://github.com/kottans/frontend-2022-homeworks/pull/400/files#diff-5acd093c4ee3aca60811a151969e036db3e07bb9ee1c3848f8a64b3f18aa8dceR27

lysenko-sergey-developer · 2022-09-15T19:12:48Z

submissions/Nik3264/js-dom/style.css

+}
+
+:root {
+  --color-header: #8d7361;


Bad naming for colours. Remove color prefix and colour must mean colour. Like space-grey, lime-green, hot-pink or just white.

lysenko-sergey-developer · 2022-09-15T19:17:13Z

submissions/Nik3264/js-dom/index.html

+    <meta charset="UTF-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Document</title>


Change the title name to something more appropriate for this page

lysenko-sergey-developer · 2022-09-15T19:22:57Z

submissions/Nik3264/js-dom/index.html

+    <main class="wrap">
+        <nav class="menu">
+            <div class="menu__item">Led Zeppelin</div>
+            <div class="menu__item menu__item__active">Beatles</div>


Beatles has menu__item__active by default but after script is inited its flushed.

I missed this. At first I tried this in the html, and then through the function in js. And I forgot to delete it in html ))

lysenko-sergey-developer · 2022-09-15T19:26:28Z

submissions/Nik3264/js-dom/index.js

+    );
+  }
+
+  hideMenuItemsActive();


If you will don't have any menu items with menu__item__active on init, you will not need to call this function.

it was a test run, already removed ))

lysenko-sergey-developer · 2022-09-15T19:53:39Z

submissions/Nik3264/js-dom/index.js

+  renderContent(0, contentItem);
+
+  menu.addEventListener("click", (event) => {
+    event.preventDefault();


Write why you using preventDefault and stopPropagation in this event listener.

-webkit-tap-highlight-color: rgba(0, 0, 0, 0);
On mobile, when tapping on the menu, a blue highlight appeared, I have not noticed before. But now I've decided to remove it. The search for this bug was longer in time. I tried everything to somehow intercept the event and process it specifically on the target, and not affect the menu. But the solution turned out to be simple. Now I know it))). But of course, I forgot to remove everything unnecessary. It's because of the rush, I'm trying to move on as quickly as possible))

lysenko-sergey-developer · 2022-09-15T19:55:31Z

submissions/Nik3264/js-dom/index.js

+
+  const menuItem = document.querySelectorAll(".menu__item"),
+    menu = document.querySelector(".menu"),
+    contentItem = document.querySelector(".item");


item is bad naming for this class

I have renamed on to group-info, and all following classes are group-info__header, group-info__src, etc..
Can it be like that?

lysenko-sergey-developer · 2022-09-15T19:55:41Z

submissions/Nik3264/js-dom/index.js

+      paragraph = document.createElement("p");
+
+    parent.innerHTML = "";
+    parent.appendChild(


appendChild support appending multiple nodes by one call

I replaced appendChild for append, and now appending all nodes with one call. AppendChild does not allow this, at least I did not find it in the documentation. But I did know the difference between append and appendChild ))

lysenko-sergey-developer · 2022-09-15T19:56:35Z

submissions/Nik3264/js-dom/index.js

+    console.log(event.target);
+    const target = event.target;
+
+    if (target && target.classList.contains("menu__item")) {


In which case you will not have event.target?

There is always a target when a click occurs. Here the condition is redundant. in my opinion. I correct it
if (target.classList.contains("menu__item"))

lysenko-sergey-developer · 2022-09-15T19:59:46Z

submissions/Nik3264/js-dom/style.css

+  --color-yellow: #c9bbae;
+}
+
+@keyframes fade {


Drop this animation fade and fade-text. If you want cool animation for this block. Try to implement fade which will be fading by vertical or "fade down".

I'm stuck on this animation. It would be easier to remove it immediately. And spend more time on the code)))
To be honest, I like learning js more than css.

I forgot about this one. Change translateX(100px); to translateY(-25px); and opacity: 0; to opacity: 0.75; Don't forget about 100% step of animation there translateX replace to 'translateY`

lysenko-sergey-developer · 2022-09-15T20:19:08Z

UI/UX Improvements:

Fade still exist. Try to replace it with 'fade down' or vertical fade.
White space on the bottom. Remove it.
Mobile version still looks bad :( I recommend adding a hamburger menu variation for the mobile version.

lysenko-sergey-developer · 2022-09-17T10:10:50Z

submissions/Nik3264/js-dom/style.css

+.group-info__text {
+  flex-grow: 2;
+  font-size: 1.5em;
+  animation: fade-text 0.85s ease-out;


OK, let's correct this. Change to
fade-text 0.7s ease-out;

lysenko-sergey-developer · 2022-09-17T10:11:03Z

submissions/Nik3264/js-dom/style.css

+  text-transform: uppercase;
+  margin-bottom: 25px;
+  text-shadow: 2px 2px 2px var(--grey-blue);
+  animation: fade-text 0.5s ease-out;


OK, let's correct this. Change to
fade-text 0.7s ease-out;

lysenko-sergey-developer · 2022-09-17T10:11:17Z

submissions/Nik3264/js-dom/style.css

+  margin-right: 20%;
+  border-radius: 2%;
+  box-shadow: 5px 15px 10px 3px;
+  animation: fade 0.7s;


OK, let's correct this. Change to
fade 0.7s ease-out;

lysenko-sergey-developer · 2022-09-17T10:13:23Z

submissions/Nik3264/js-dom/style.css

+  0% {
+    transform: translateY(-50px);
+    opacity: 0;
+    -webkit-clip-path: polygon(100% 0, 100% 100%, 0 100%, 0 100%);


Remove clip-path.
Change translateY(-50px); to translateY(-25px);
Change opacity: 0; to opacity: 0.75;

lysenko-sergey-developer · 2022-09-17T10:13:25Z

submissions/Nik3264/js-dom/style.css

+    clip-path: polygon(100% 0, 100% 100%, 0 100%, 0 100%);
+  }
+  100% {
+    transform: translateY(0);


Remove clip-path.

lysenko-sergey-developer · 2022-09-17T10:14:36Z

submissions/Nik3264/js-dom/style.css

+  }
+}
+
+@media screen and (max-width: 480px) {


Mobile version should start from 768px

lysenko-sergey-developer · 2022-09-17T17:07:53Z

@Nik3264 Looks like need one small fix to image animation and we did it!

Nik3264 · 2022-09-17T18:01:39Z

Thanks for your time spent for me! I feel like I still have a lot to learn. But this practice is much more than just reading documentation. I really appreciate this!

lysenko-sergey-developer

LGTM! 😸

lysenko-sergey-developer · 2022-09-17T18:43:12Z

@Nik3264 Congratulation! You really did a lot! I enjoyed your final page variant, so cool!

My PR width DOM-api

31df08c

A-Ostrovnyy added the task-DOM label Sep 2, 2022

lysenko-sergey-developer requested changes Sep 14, 2022

View reviewed changes

removed short-lived blue background when clicking on mobile, reduced …

358a27f

…space between header and img, added function render, renamed array

Nik3264 requested a review from lysenko-sergey-developer September 15, 2022 11:04

lysenko-sergey-developer self-assigned this Sep 15, 2022

lysenko-sergey-developer requested changes Sep 15, 2022

View reviewed changes

fixed bugs and added burger-menu

da5625b

Nik3264 requested a review from lysenko-sergey-developer September 16, 2022 18:24

lysenko-sergey-developer requested changes Sep 17, 2022

View reviewed changes

changed animation, screen size in media

2f7a6a5

Nik3264 requested a review from lysenko-sergey-developer September 17, 2022 14:26

some changes with img animation

53a6f84

changed X to Y

88a1130

lysenko-sergey-developer approved these changes Sep 17, 2022

View reviewed changes

lysenko-sergey-developer merged commit d3d37e9 into kottans:main Sep 17, 2022

My PR width DOM-api #400

My PR width DOM-api #400

Conversation

Nik3264 commented Sep 2, 2022

Document Object Model

github-actions bot commented Sep 2, 2022

Nik3264 commented Sep 2, 2022

Nik3264 commented Sep 4, 2022

lysenko-sergey-developer commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysenko-sergey-developer commented Sep 14, 2022

Nik3264 commented Sep 15, 2022

lysenko-sergey-developer commented Sep 15, 2022

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

lysenko-sergey-developer commented Sep 15, 2022

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

lysenko-sergey-developer commented Sep 17, 2022 • edited Loading

Nik3264 commented Sep 17, 2022

lysenko-sergey-developer left a comment

Choose a reason for hiding this comment

lysenko-sergey-developer commented Sep 17, 2022 • edited Loading

lysenko-sergey-developer commented Sep 17, 2022 •

edited

Loading

lysenko-sergey-developer commented Sep 17, 2022 •

edited

Loading