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

PR Estefanny Project week 3 business website #372

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

Conversation

FannyEste
Copy link

No description provided.

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Hi Estefanny, excellent work on your business site! 🍷

HTML

  • Great use of a video in the header (<video autoplay muted loop>), which creates an engaging first impression. You also provide an overlay with a clear call-to-action (CTA) button, making it easy for users to navigate to the sign-up form 🥳
  • Nice usage of the required attribute for some basic validation!
  • When the label is not wrapping the input, we need to add the for attribute to the label, and the id attribute to the input:
<label for="name">First Name</label>
<input
  required
  id="name"
  type="text"
  placeholder="First Name"
  name="name
/>

CSS

  • The color scheme is well thought out, with yellow and black creating a strong contrast, especially in the footer. Love the yellow text on the images 🤩
  • Try to always keep the styling to the CSS file, i.e. instead of using the ´b´ tag, add some rules for font-weight in the CSS.
  • The hover effect on the button reduces the opacity significantly (opacity: .6). Consider lowering it slightly less (e.g., opacity: .8) to maintain better readability.
  • I strongly suggest to give the form more space in mobile, to make it less cramped.
    Skärmavbild 2024-09-04 kl  10 55 12

Clean Code

  • Make sure you have proper indentation throughout your HTML file. Children should be indented one level (and one level only), such as:
<div class="image-gallery">
  <img src="./images/our-wine.png" alt="our-wine" class="gray-image">
  <img src="./images/month-wine.png" alt="month-wine">
  <img src="./images/next-events.png" alt="events">
</div>

and siblings should have the same level of indentation, such as:

<label></label>
<input/>

We'll allow it this time, but please think about this going forward as it will make your code much easier to read and maintain.

  • Remove unused code (e.g. the end of your CSS file)

You've done an amazing job! A few tweaks related to clean code and mobile optimization will make this project even more polished. Keep up the good work 🚀

Copy link

@ElinaEH ElinaEH left a comment

Choose a reason for hiding this comment

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

Well done Estefanny on creating such a nice wine business site!
I really like the title/name "wine me". Overall I think your project and your code is well structured with a great use of names and comments. Your deployed site looks great such as all the images and code. Your media queries also makes the page look good in different sizes and devices. I tried your sign up form and filled in some details, which worked perfectly.

Once again, well done Estefanny 👯

code/index.html Outdated
<!---hero section-->
<div class="hero-container">
<!---Hero video-->
<video autoplay muted loop id="hero-video">
Copy link

Choose a reason for hiding this comment

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

Very nice video that works well on the deployed link and fits the theme perfect!

code/index.html Outdated
<input type="email" placeholder="Email" name="email" required>

<label>
<input type="checkbox" name="newsletter" style="margin-bottom: 15px;"> Sign me up for the newsletter
Copy link

Choose a reason for hiding this comment

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

Here it would be nice to have the same indentation as the other rows, it looks like "me up for the newsletter" is a bit outside the rest of the code. You probably have the space 2 indentation on and I'm not sure how to do it but something we can research for the future :)

code/index.html Outdated
<input type="checkbox" name="newsletter" style="margin-bottom: 15px;"> Sign me up for the newsletter
</label>

<p>By creating an account you agree to our <a href="#" style="color: dodgerblue">term and conditions</a>.</p>
Copy link

Choose a reason for hiding this comment

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

The same thing I mentioned on row 74 I think could be applicable here as well.

border-radius: 8px;
}

button:hover {
Copy link

Choose a reason for hiding this comment

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

Really nice with the hover effect here! I haven't used the this effect in my CSS yet but will definitely
take som inspiration from you here!

code/style.css Outdated
}

.logo-container img {
height: 50px; /* Ajusta la altura de la imagen según sea necesario */
Copy link

Choose a reason for hiding this comment

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

I can see some of your comments are in spanish here. Maybe it's good to do them all in english :)

}



Copy link

Choose a reason for hiding this comment

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

I tried your media queries you've written here on your deployed site and it worked really well! Good job :)

opacity: .6;
transition: .3s;
}

Copy link

Choose a reason for hiding this comment

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

There's some styling elements here I haven't tried before which sounds really interesting! Will try it out on my next project.

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