My awsome bike #2572

DominikaKarmel · 2023-07-21T14:27:00Z

https://DominikaKarmel.github.io/layout_miami/

darokrk

UI looks awesome 🎉 but code needs improvements 👨🏽‍💻

darokrk · 2023-07-25T06:51:17Z

src/styles/blocks/logo.scss

+@import "utils/mixins.scss";
+@import "utils/reset.scss";
+
+.logo {


There is an extra empty line after the '.logo' class declaration. Consider removing unnecessary empty lines for cleaner code.

darokrk · 2023-07-25T06:51:20Z

src/styles/main.scss

-body {
-  background: $c-gray;
-}
+@import "utils/variables.scss";


Importing 'variables.scss', 'mixins.scss', and 'reset.scss' at the end of the file. They should be imported at the beginning of the file as they contain base styles, variables, and mixins that need to be applied first and may be used in other stylesheets.

darokrk · 2023-07-25T06:51:27Z

src/index.html

+    <title>MyBike</title>
+    <link
+    rel="icon"
+    type="image"


The type attribute for the favicon link should be 'image/x-icon' or 'image/vnd.microsoft.icon' for .ico files. If you're using .jpg or .png, it should be 'image/jpeg' or 'image/png' respectively.

darokrk · 2023-07-25T06:51:27Z

src/index.html

@@ -1,13 +1,555 @@
 <!DOCTYPE html>
-<html lang="en">
+<html lang="en" class="page">


You added a class to the html tag. It's generally not recommended to add classes to html tag. Consider moving this to body tag or another wrapper div.

darokrk · 2023-07-25T06:51:34Z

src/styles/blocks/form-field.scss

+
+  &:hover {
+    border-color: #d5d7de;
+    color: $formColor;


this variable should point color, name can be e.g $color-black or $color-white, know that is used here for form but maybe you will can use this somewhere else

darokrk · 2023-07-25T06:58:11Z

src/styles/blocks/button.scss

+
+  width: 100%;
+  height: 56px;
+


please remove those white spaces in whole project

darokrk · 2023-07-25T06:59:09Z

src/styles/blocks/button.scss

+  font-size: inherit;
+  font-weight: 700;
+  line-height: 27px;
+  color: $DarkTextColor;


this variable can be just $color-dark

darokrk · 2023-07-25T06:59:20Z

src/styles/blocks/button.scss

+  width: 100%;
+  height: 56px;
+
+  background: #fff;


consider to move this color to variables

darokrk · 2023-07-25T07:00:08Z

src/styles/blocks/contact-us.scss

+    margin-bottom: 8px;
+
+    font-size: 14px;
+    line-height: $textLineHeight;


maybe cleaner $text-line-height

darokrk · 2023-07-25T07:04:12Z

src/styles/blocks/header.scss

+    justify-content: space-between;
+
+    height: 100vh;
+    padding: $headerTopPadding 0 88px;


this variable can be just $header-padding

darokrk

The code still needs some refactoring. Specially BEM naming classes, you have a lot of mix there, please try to refactor my mentions and should be better 👍🏽 😃

darokrk · 2023-08-01T05:48:14Z

src/index.html

+                <img
+                  src="images/icons/menu_logo.png"
+                  alt="MyBike logo"
+                  class="logo--image"


this should be rather .logo__image

darokrk · 2023-08-01T05:49:13Z

src/index.html

+                <img
+                  src="images/icons/menu_logo.svg"
+                  alt="MyBike logo"
+                  class="logo--image"


this should be rather .logo__image

darokrk · 2023-08-01T05:49:32Z

src/index.html

+
+              <a
+                href="tel: +1 234 555-55-55"
+                class="icon icon--phone header__call"


icon__phone

darokrk · 2023-08-01T05:49:41Z

src/index.html

+              ></a>
+              <a
+                href="#menu"
+                class="icon icon--menu"


darokrk · 2023-08-01T05:50:27Z

src/index.html

+                >
+              </a>
+
+              <a href="#" class="icon icon--cross"></a>


icon__cross

darokrk · 2023-08-01T06:01:17Z

src/index.html

+            </h2>
+
+            <div class="details__bike-details bike-details">
+              <div class="bike-details__photos">


bike-details__photos

darokrk · 2023-08-01T06:02:28Z

src/index.html

+                      <img
+                        src="images/photos/1.jpg"
+                        alt="detail 1"
+                        class="bike-details__photo"


bike-details__image

darokrk · 2023-08-01T06:04:50Z

src/index.html

+        <section class="page__section contact-us" id="contacts">
+          <div class="container">
+            <h2
+              class="page__section-title"


contact-us__title

darokrk · 2023-08-01T06:07:16Z

src/styles/utils/variables.scss

+$titleSizeIpad: 48px;
+$titleSizeDesktop: 64px;
+$textLineHeight: 140%;
+$DarkGrayColor: #292929;


names for colors should follow this convention:

$color-dark-gray;
$color-white;
$color-light-gray;

it's shorter and easier to read :)

darokrk · 2023-08-01T06:08:35Z

src/styles/utils/variables.scss

@@ -0,0 +1,8 @@
+$headerPadding: 30px;


think you should follow naming like that:

$header-padding;
$title-size-ipad;
$title-size-desktop;
etc.....

it's easier to read compare to camelcase naming :)

Radoslaw-Czerniawski

Layout looks very good :D, there are some errors though:

16.Add a smooth scroll for the whole page, Checklist
you are doing a lot of unnecessary nesting, your html structure should be as flat as possible, one wrapper per pair / set of children should be enough, add reusable classes to already existing wrappers rather than creating more
you are not using semantic html, there is a lot of text wrapped in divs rather than in span / p
I didn't point out all error regarding the nesting and wrong semantics.

Radoslaw-Czerniawski · 2023-08-15T13:04:10Z

src/index.html

-  <body>
-    <h1>Miami</h1>
+  <body class="page">
+    <div class="page__body">


This class name suggest body tag, change it to something like page__wrapper

Radoslaw-Czerniawski · 2023-08-15T13:06:37Z

src/styles/blocks/page.scss

+  font-family: Poppins, sans-serif;
+  font-size: 16px;
+  color: $color-white;
+  scroll-behavior: smooth;


This should be set on the html element, add it in the main.scss after imports

Radoslaw-Czerniawski · 2023-08-15T13:07:12Z

src/styles/blocks/page.scss

+    overflow: hidden;
+  }
+
+  &__body {


As mentioned previously, by reading the scss alone I immediately assume this styles body element

Radoslaw-Czerniawski · 2023-08-15T13:22:29Z

src/index.html

+    <div class="page__body">
+      <div class="upward"></div>
+
+      <header class="header">


Why is there a triple nesting here? One container per pair of children is more than enough:

<header> <a><img></a> <h1>...</h1> </header>

Radoslaw-Czerniawski · 2023-08-15T13:29:44Z

src/index.html

+        </div>
+      </header>
+
+      <nav class="page__menu menu" id="menu">


Again, why there is a need for such deep nesting? Apart from the classes this should be closer to such structure,

also nav is placed in the header area so logically it should be a part of the header

Radoslaw-Czerniawski · 2023-08-15T13:30:45Z

src/index.html

+              </div>
+
+              <div class="grid__item grid__item--ipad--4-6 grid__item--desktop--7-12">
+                <div


This should be a p

Radoslaw-Czerniawski · 2023-08-15T13:31:00Z

src/index.html

+                    Sporty 4
+                  </h3>
+
+                  <div class="bike__description">


Radoslaw-Czerniawski · 2023-08-15T13:31:06Z

src/index.html

+                    The iconic frame brought to a new performance height as a sporty, active ride.
+                  </div>
+
+                  <div class="bike__info bike__info--rotate">


Radoslaw-Czerniawski · 2023-08-15T13:31:17Z

src/index.html

+                    Ride in town ST
+                  </h3>
+
+                  <div class="bike__description">


Radoslaw-Czerniawski · 2023-08-15T13:47:48Z

src/index.html

+              </div>
+            </div>
+
+            <div class="bike-details__info bike-details">


darokrk

Nice fixes but what about Radek's mentions? Did you see them? Please try to fix them and correct demo link bcs it does not work now, so can't see how page looks like live 🛠️

DominikaKarmel · 2023-10-17T14:29:00Z

Nice fixes but what about Radek's mentions? Did you see them? Please try to fix them and correct demo link bcs it does not work now, so can't see how page looks like live 🛠️

done! commit before this one was mistake ;)

loralevitska

You did an amazing job! Looks good! Let's do some improvements (check comments)

center your content to avoid such view on bi screens
download another image from figma without additional elements
consider remove background-color after autocomplete the form, change the default styles because it breaks design. Read more about changing autocomplete styles
make all form field required (message)
fix contact part according mockup
your solution

design
cross icon in menu should have '#1d1d1d' color according design

anastasiiavorobiova

Your work looks stunning! I love your animations

Consider fixing validation errors

The menu for mobiles also should be fixed

Rounded corners don't meet the Figma design

Add a placeholder to the message field

DominikaKarmel · 2023-10-18T21:04:53Z

I fixed space for href on an 'a' elements,
I don't know, why should the menu panel be longer than the header? Why it can't be 100vh? I set the height like figma design, but i don't know how should I fix it for cell phones with screen rotated...
I want the border-radius for the images in the 'details' section to remain as it is. I know that it is different from the figma, while the task says that the layout does not have to match every pixel. Now it is nice :)
I donno what happend with placeholder in textarea and now i don't know how to fix it. It is correct in html, in css also - i think. The problem is, that when you click it cursor appears in middle of window and you have to hold backspace to see placeholder. Trying to fix it, but i don't have any idea what happend.

volodymyr-soltys97

Nice job 👍
Very nice animations for content 🚀
Let's improve your page

It is not necessary to hardcode the height of the header and menu, it should be the entire height of the screen, i.e. 100%

Add a placeholder text for the textarea and need to fix the indents for text

Need to remove autocomplete styles for these fields

When I send data no need to show this data in the url, the fields should only clear and the page should not reload

Add hover effects for these clickable elements

Need to fix the background image size on the footer
Demo

Design

DominikaKarmel · 2023-10-19T10:51:52Z

ad. p. 4 @volodymyr-soltys97
You meant i should chenge type of button on 'reset'? Done, but will it be ok if the recruiter will see the type 'reset' instead of 'submit' in my html next to the 'Send' button?

anastasiiavorobiova

You have a huge progress here! There are just a few fixes from previous reviews still here:

Validation errors should be fixed

Links should slightly change their styles on hover

These form fields should be required

A user should be able to scroll the menu in landscape mode (checklist - 17)

For resetting values in the form on submit, you can use the button type submit and this small piece of code

Viktor-Kost

Nice!

To improve:

Use svg format instead of png one for icons all over the page
Add transition for links hover effects
Use common font for all inputs

You have nice section animations yet it's not user friendly when these animations invoke each time when they got into viewport. Try to trigger them only once

volodymyr-soltys97

Well done 🔥

DominikaKarmel added 7 commits July 18, 2023 14:49

header

f027f5f

section about

b1c59e2

section compare & details

8e1b966

form-field & footer

66a125f

working nav

309db5f

my awsome bike

dc0bdbf

fix unnecessary classes

10a734f

darokrk suggested changes Jul 25, 2023

View reviewed changes

my awsome bike

3b86cf3

DominikaKarmel requested a review from darokrk July 31, 2023 13:21

my awsome bike

6c3422f

darokrk suggested changes Aug 1, 2023

View reviewed changes

my awsome bike

44e265e

DominikaKarmel requested a review from darokrk August 10, 2023 11:31

Radoslaw-Czerniawski suggested changes Aug 15, 2023

View reviewed changes

darokrk suggested changes Sep 13, 2023

View reviewed changes

DominikaKarmel added 5 commits September 19, 2023 10:55

reanem class, add scroll-behavior

06552e4

page__wrapper

d5ace4e

still in progress

9b087ca

removal of unnecessary classes

16c8327

my awsome bike

577f444

DominikaKarmel requested a review from darokrk October 17, 2023 14:22

loralevitska suggested changes Oct 17, 2023

View reviewed changes

my awsome bike

a96eaf3

DominikaKarmel requested a review from loralevitska October 18, 2023 09:19

anastasiiavorobiova suggested changes Oct 18, 2023

View reviewed changes

my awsome bike

3710e46

DominikaKarmel requested a review from Radoslaw-Czerniawski October 18, 2023 21:15

volodymyr-soltys97 suggested changes Oct 19, 2023

View reviewed changes

my awsome bike

f03cc76

DominikaKarmel requested a review from volodymyr-soltys97 October 19, 2023 11:00

anastasiiavorobiova suggested changes Oct 19, 2023

View reviewed changes

my awsome bike

2cae6ba

DominikaKarmel requested a review from anastasiiavorobiova October 19, 2023 13:49

Viktor-Kost suggested changes Oct 19, 2023

View reviewed changes

my awsome bike

4e6878c

DominikaKarmel requested a review from Viktor-Kost October 19, 2023 21:37

volodymyr-soltys97 approved these changes Oct 20, 2023

View reviewed changes

My awsome bike #2572

Are you sure you want to change the base?

My awsome bike #2572

Conversation

DominikaKarmel commented Jul 21, 2023

darokrk 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

darokrk 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

Radoslaw-Czerniawski 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

darokrk left a comment

Choose a reason for hiding this comment

DominikaKarmel commented Oct 17, 2023

loralevitska left a comment

Choose a reason for hiding this comment

anastasiiavorobiova left a comment

Choose a reason for hiding this comment

DominikaKarmel commented Oct 18, 2023

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

DominikaKarmel commented Oct 19, 2023

anastasiiavorobiova left a comment

Choose a reason for hiding this comment

Viktor-Kost left a comment • edited Loading

Choose a reason for hiding this comment

Nice!

To improve:

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

Viktor-Kost left a comment •

edited

Loading