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

Annas business site #381

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

Conversation

Anna2024WebDev
Copy link

No description provided.

@Anna2024WebDev
Copy link
Author

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 Anna! Great job with your business site! Some feedback coming up:

HTML

  • The HTML structure is solid, and you’ve included essential elements like the <header>, <main>, and <section>. Just remember that the header is usually not part of the main element, but rather a sibling to the main element, as well as the footer. So move the header up from the main, and move the footer into the body.
  • Nice usage of attributes for the video element!
  • Well structured form ⭐ I noticed you have an extra form tag within your form, so please get rid of that

CSS

  • Maybe you'd want to add some padding around the footer? Especially in mobile, it looks a bit cramped.
  • The link is not really visible in my Google Chrome browser. Consider increasing the contrast by choosing another color:
    Skärmavbild 2024-09-04 kl  14 20 55

Your project is coming along really well, just some small tweaks to improve. Keep up the good work ⭐

Copy link

@Fannyhenriques Fannyhenriques left a comment

Choose a reason for hiding this comment

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

Amazing work on your website, Anna! 🌟

Everything seems to be functioning well and is responsive across all devices—well done! I love the video and how you've positioned the header text, it creates a very inspiring vibe. 😊

The form is nicely styled, and it works perfectly! Great job! Your code is also really clean, easy to read, and follow!

Special shoutout for embedding the filter: brightness attribute directly within the hero-video. That’s a smart move—it keeps the code cleaner, whereas I had separated mine into its own class. Great work! 👏

Areas for Improvement:
I've left some comments in the "files changed" section for easier reference, but I'll outline them here as well. 😊

HTML:

  • I noticed there is an extra

    tag nested within the existing element in the signup section. You can remove that extra tag to avoid redundancy.

  • Input type:
    I received feedback on my project regarding the type attributes for inputs, and I believe you could apply the same improvement. The type attribute values for the inputs should be updated to text for better accuracy:

Like this:
<input type="text" id="company"name="company" placeholder="Company" required />

CSS:

  • It looks like there are two styles for the button: one under .signup button and another for button. Since they seem to overlap, I would consider removing the redundant button class styling if it’s not needed.

Overall, you’ve done an incredible job on this website! The required fields are all in place, and I love the video, the color scheme, and the overall styling. Fantastic work!! 🎉

code/index.html Outdated

<section class="signup">
<form action="https://httpbin.org/anything" method="POST">
<form>

Choose a reason for hiding this comment

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

Here is an extra

tag nested within the existing element in the signup section. You can remove this extra tag to avoid redundancy.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Received the same input from Matilda as well, thanks for noticing! It is removed.

height: auto;
width: 100vw;
max-height: 100vh;
filter: brightness(65%);

Choose a reason for hiding this comment

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

Embedding the filter: brightness attribute directly within the hero-video. A really clever move!!

}


.hero-text {

Choose a reason for hiding this comment

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

Nice placement of the hero text!! Looks really good!

code/index.html Outdated
<label for="name"></label>
<input type="name" id="name" name="name" placeholder="Name" required />
<label for="company"></label>
<input type="company" id="company" name="company" placeholder="Company" required />

Choose a reason for hiding this comment

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

The type attribute values for the inputs should be updated to text for better accuracy.
Like this:
<input type="text" id="company"name="company" placeholder="Company" required />

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! Very good and correct point indeeed! Updated now.

code/style.css Outdated
}


button {

Choose a reason for hiding this comment

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

this styled button might be redundant since you have a .signup button above. If unnecessary I would consider removing it. ☺️

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @Fannyhenriques for noticing! I didn´t notice this before that I had forgotten to remove the previous styling of the button. Now It is removed.

…ra form tag and added overflow x-hidden to the main section
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