Skip to content

MyBike page#2741

Open
Darmi555 wants to merge 9 commits into
mate-academy:masterfrom
Darmi555:develop
Open

MyBike page#2741
Darmi555 wants to merge 9 commits into
mate-academy:masterfrom
Darmi555:develop

Conversation

@Darmi555

Copy link
Copy Markdown

@natalia-klonowska natalia-klonowska left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

your demo link doesn't work. Please deploy your project using npm run deploy command

@natalia-klonowska natalia-klonowska left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • buttons should have the same font size and resize in the same way when screen width changes
Image
  • sometimes a small gap appears between sections so it would be better to set background color for main instead of section
Image
  • phone number should be displayed in single line if there is enough space
Image

CHECKLIST:

  • Change text color on hover for phone, email and address
  • When you click on phone icon or phone number in contacts section, make sure that there is no 404 error, make it a real link to start a call on device
  • When clicking on any location / address - prevent errors and make it to open location in Google Maps
  • apply validation of the form fields (required, email / tel etc.), then it is clear in what format to enter the data
  • Form shouldn't be submitted if some of the fields are not filled

@natalia-klonowska natalia-klonowska left a comment

Copy link
Copy Markdown

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 small fixes are needed:

  • images should be the same height
Image
  • on desktop and tablet versions button should be shorter
Image

Comment thread index.html Outdated
<link rel="icon" type="image/png" href="photos/favicon.svg">
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin="anonymous">
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@100..900&display=swap" rel="stylesheet">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you should use Poppins font instead of Roboto

Image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you’re still using Roboto font. change it to Poppins

Comment thread index.html Outdated
<link rel="icon" type="image/png" href="photos/favicon.svg">
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin="anonymous">
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@100..900&display=swap" rel="stylesheet">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you’re still using Roboto font. change it to Poppins

@natalia-klonowska natalia-klonowska left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Roboto is still applied in demo because css file hasn’t been updated. also remember that this project supports scss so there’s no need to include css files just update html to reference scss instead of css

Image

@Darmi555

Copy link
Copy Markdown
Author

The deploy page didn't work with SCSS in HTML, so I just updated the CSS and left it at that. I'm glad you said 'there is no need to include CSS files' rather than 'you can't include CSS files', right? :D

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.

2 participants