Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eco landing page #386

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

HigorPedroso
Copy link

Copy link

@pedro-ruas pedro-ruas left a comment

Choose a reason for hiding this comment

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

Some really minor changes in formatting

src/index.html Outdated
type="tel"
name="number"
placeholder="phone number"
required pattern="^\+380\d{9}$"

Choose a reason for hiding this comment

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

"required" should be in one line, and "pattern" in other.
Just identation fixing

src/index.html Outdated
type="text"
name="name"
placeholder="Your name"
required pattern="\w+"

Choose a reason for hiding this comment

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

Add a line break between required and pattern

src/index.html Outdated
type="email"
name="email"
placeholder="e-mail"
required class="form__input input"

Choose a reason for hiding this comment

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

Add a line break between required and class

}
}

@import 'sections/header';

Choose a reason for hiding this comment

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

Move all these imports to the file begining, please

Copy link

@EdPirro EdPirro left a comment

Choose a reason for hiding this comment

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

Hey, @HigorPedroso! Sorry for the delay on this review, but good job. I pointed out just a few minor improvements, and it should be good to go! 👏

src/index.html Outdated
</li>

<li class="menu__item">
<a href="#contact" class="menu__link">Contact</a>
Copy link

Choose a reason for hiding this comment

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

This links to #contact but your contact block has id="contacts", ending with s.

</li>

<li class="nav-footer__item">
<a href="#contact" class="nav-footer__link link-hover">
Copy link

Choose a reason for hiding this comment

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

This links to #contact but your contact block has id="contacts", ending with s.

src/index.html Outdated
LEARN MORE
</a>

<a href="#shop" class="section__button button">
Copy link

Choose a reason for hiding this comment

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

This link should probably reference #contacts and not #shop

src/index.html Outdated
<input
type="tel"
name="number"
placeholder="phone number"
Copy link

Choose a reason for hiding this comment

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

Placeholder should be a sample of a valid value for this input (specially when you have custom validation patterns)

src/index.html Outdated
name="name"
placeholder="Your name"
required
pattern="\w+"
Copy link

Choose a reason for hiding this comment

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

This pattern restricts the input to be a single word without spaces; it also accepts numbers and underscores. Perhaps [A-Za-z ]*[A-Za-z] would be more fitting.

Copy link

@jv-aquino jv-aquino left a comment

Choose a reason for hiding this comment

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

Great job Higor! Just a tip: remember about object-fit for imgs
image

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.

4 participants