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

Project business site #362

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

Project business site #362

wants to merge 24 commits into from

Conversation

joheri1
Copy link

@joheri1 joheri1 commented Aug 26, 2024

No description provided.

@joheri1 joheri1 changed the title Testing testing Project business site Sep 1, 2024
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.

Good job with this project Johanna, it's well structured and I appreciate reading your reflections in the README.

HTML

  • Great job using the required attribute on the inputs. This ensures that users cannot submit the form without filling out these fields.
  • Great use of semantic elements, such as main and footer (just make sure the footer is within the body element). Consider using a header instead of a div for a more semantic approach.

CSS

  • Excellent use of max-width: 100%; and object-fit: cover; for images. This ensures that your images scale correctly and maintain their aspect ratio across different screen sizes.
  • Make sure that the form elements (inputs, buttons) are styled to match the overall aesthetic of the page. Aligning the name inputs would be nice! Reach out on Stack Overflow if you want tips on how to achieve it 😊
  • Your CSS seems well-prepared for responsive design, but make sure that all elements, especially the form, adjust properly on smaller screens. You may want to use media queries to fine-tune e.g. the font-size of the placeholders for smaller screens.

Clean Code

  • Make sure to properly indent your code, each new element should have it's own line:
<label>
  <input type="radio" name="Agree" required>
  Agree
</label>

and even better, if each attribute gets its own line to improve readability:

<label>
  <input
    required
    type="radio"
    name="Agree"
  />
  Agree
</label>

You’re doing a fantastic job! Just a few small tweaks, like refining the radio button setup, will make your project even better.

Keep up the great work 🎉 ​

The biggest issue with this was that I found it hard to understand/read the code in the inspector, so I had to google a lot. How to add the text inside of each input field ("placeholder"), how to add space around the boxes, etc. The form also contained things I couldn't understand, because they weren't visible to the eye, such as "<label for="frm_email_3">Om du är mänsklig, lämna det här fältet tomt.</label>".

3. Minor layout issues.
The name fields are not positioned the way I wanted, the radio button is oval and can't be unchecked, etc. Since time was running out, I started to write PostIts with all the flaws I wanted to correct, added some in a pile that need to be fixed before handing in my project (upload to Netlify, make the site responsive for different devices, etc), and some notes in a pile to fix later, if I have the time (like the oval radio button).
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the behaviour of radio buttons, it's a group of buttons and you always have to pick exactly one, i.e.
Are you a:

  • Lunch Learner
  • Early Bird
  • Evening Owl

We know that each user completing our form will be one - and one only - of these options. For checking/unchecking, it's better to use a checkbox. Checkboxes can be grouped as well, but for when the users can choose more than one option, i.e.
Are you interested in:

  • Frontend development
  • Backend development
  • UX
  • UI

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