Skip to content

MyBike landing page - Portfolio project#2710

Open
folg-code wants to merge 48 commits intomate-academy:masterfrom
folg-code:develop
Open

MyBike landing page - Portfolio project#2710
folg-code wants to merge 48 commits intomate-academy:masterfrom
folg-code:develop

Conversation

@folg-code
Copy link

@folg-code folg-code commented Sep 20, 2025

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • burger menu icon changes size on hover which seems unintentional
  • checklist: Page shouldn't be reloaded on form submit
  • change background image position and size of icons to match design:
image
  • menu links should be in uppercase, last two links are missing styles and close icon size is wrong
image

src/index.html Outdated
<nav class="section section__content navigation">
<a
href="index.html"
class="navigation--logo"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEM : "Using a Modifier Instead of an Element"/"Using a Modifier Without the Belonging Class"

fix all other instances of this mistake

</head>
<body>
<h1>Miami</h1>
<body class="body">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use tag body for styling or create file for block body for consistency ;)

src/index.html Outdated
Comment on lines +146 to +151
<main class="main">
<section
class="section section__content"
id="about-us"
>
<h2 class="section__title title title--section main__title">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEM: "Using an Element Inside Another Block"

-fixing issues :
"Using a Modifier Instead of an Element"/"Using a Modifier Without the Belonging Class"
filip added 2 commits September 23, 2025 21:41
- add href to phone number
@folg-code
Copy link
Author

Sorry for commits after request review.
There was some new issues beacause of reworks.
I fixed it all and page is ready to request

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use a full-size background image and apply styling (background-position, background-size) so the image adapts better between breakpoints

  • layout is not responsive it's fixed with breakpoints which results in too large margins between them:

image

Here you can find a comparison between fixed and responsive layouts and here you can see example of responsive layout with breakpoints

image
  • delete italic font style from contact links
  • textarea should have different border-radius
  • check and correct space between elements
  • buttons send and explore shouldn't be in uppercase
image
  • links should be in uppercase
  • underline for the last link should be lower
  • change number to match the design

Comment on lines +15 to +23
&--phone {
width: 24px;
height: 16px;

@include on-mobile {
width: 18px;
height: 12px;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Suggested change
&--phone {
width: 24px;
height: 16px;
@include on-mobile {
width: 18px;
height: 12px;
}
}
&--phone {
width: 24px;
height: 24px;
@include on-mobile {
width: 18px;
height: 18px;
}
}

Comment on lines +24 to +35
.navigation__menu {
width: 24px;
height: 16px;

@include on-mobile {
width: 18px;
height: 12px;
}

background-image: url('../images/menu.svg');
background-repeat: no-repeat;
background-size: contain;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change height to match width and add background-position to properly center burger menu icon with the other navigation icons

Suggested change
.navigation__menu {
width: 24px;
height: 16px;
@include on-mobile {
width: 18px;
height: 12px;
}
background-image: url('../images/menu.svg');
background-repeat: no-repeat;
background-size: contain;
.navigation__menu {
width: 24px;
height: 24px;
@include on-mobile {
width: 18px;
height: 18px;
}
background-position: center;
background-image: url('../images/menu.svg');
background-repeat: no-repeat;
background-size: contain;

filip added 2 commits September 29, 2025 14:03
…ppercase, font style in contact links, textarea borderradius
…ppercase, font style in contact links, textarea borderradius
Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 🚀 Just a few final things to fix:

  • change italic font style and add line-height for address link
image image
  • add letter-spacing style, font-size change from 14px to 12px and reduce gap to phone icon to 20px
image

src/index.html Outdated
<div class="menu__content">
<nav class="navigation">
<a
href="home"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 error

Suggested change
href="home"
href="#home"

src/index.html Outdated
href="./styles/main.scss"
/>
<link
rel="favicon"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rel="favicon"
rel="icon"

-favicon
-line-height and adress link font-style
-letterspacing font-size and gap to phone icon and number"
Copy link

@vsmutok vsmutok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants