mobile first huddle landing page with HTML and SASS using SCSS

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 4 years ago

  Hi Ahmed,

  As already said, the main issues here are with html semantics. I don't think it's that far off tbh. The main things which are really important are

  • logo image needs alt="Huddle"
  • other image needs alt attribute either with a description or left empty with an aria-hidden="true"
  • Register should be an anchor tag (not button) as this would take someone to a registration form/page.
  • social icons should all be in anchor tags as they would also trigger navigation (not sure why there's two versions of each icon?!)
  • all interactive elements then need clearly visible focus states (and preferably hover & active states too)

  I hope this helps you, good luck

  2

  ---

  

  Ahmed Alhalfa•40

  @ahmedalhalfaPosted almost 4 years ago

  @grace-snow thanks for the feedback grace, i made two versions because the desktop version of the site has a slightly bigger social icons and here i didn't use any js so i had to make this decision, and for the rest of the points i will surely apply them to the project.

  0

  

  T

  Grace•31,270

  @grace-snowPosted almost 4 years ago

  @ahmedalhalfa you don't need to have 2 versions though, font size would do it ☺

  1
• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  I mean in terms of how it looks, it's pretty identical so you did a great job at recreating the page!

  There's definitely some things that could be improved in your HTML along with your sass. Your markdown looks great up until the children of your 'main' element. In terms of semantics, your have divs with a classname that includes "section". You should replace those div elements with section elements. The classnames for the main element's children seem a little odd to me but that's just a preference. I feel like the "main__" prefix isn't necessary, especially if you used the section elements. Then you could have more accurate classnames like "section__images" for the first section and "section__text" for the second section.

  Some more HTML semantics; you should have an "alt" attribute on your img elements and the value should describe the image in case the image can't load. Also you really should use a button or a tag when using a button. There's really no need to make it a div, but if you do you should at least set an attribute of role = "button" on it. And a tiny little thing i noticed is that you have an h2 element with a classname saying "header". When just making a component like this it's not a big deal but when you're making big websites you should start with an h1 tag and work your way down to h6 without skipping any. The hX elements indicate hierarchy of sections in your page. And such a small critique but the classname should say "heading", not "header", since header is it's own html element that's very commonly used.

  For your styling I like your partial files they look nice and are well organized and help reduce some of your code. I like that you only included two fonts and also had a global reset for box-sizing/padding/margin. I like your use of rem units for margins and padding and it looks like your main is full height and you centered it with the 0 auto margin method which is nice. I think my biggest gripe is with your classnames having the main__ prefix. I don't see what that really adds, and it increases your sass nesting quite a bit, while also reducing the re-usability of any style blocks, which is a powerful feature of sass.

  For example if your "main__logo" class was instead "logo" (which is still not a great name), you could still access that class in the main selector by using & > .logo, because the > symbol selects the direct child of whatever selector block you're in. Even better tho, you could just extract that .logo block and put it completely outside of the main selector block so it's at the first level. You could actually extract every selector to the first level by changing the style of how you name your classes. Generally, classes will be named like that to address a change in it's state (ex: ".TextField" has a value entered into it and changes to ".TextField TextField__HasValue" or something like that).

  Personally I'd add some interactivity to the buttons, especially the div styled as a button. At least add cursor: pointer to the div button. Also for your .fab selector you can just place cursor: pointer at the first level of .fab, putting it in the hover state isn't necessary because placing the new rule would turn your mouse to a pointer when it's over the element at any point in time.

  Overall I'd say it looks quite nice, especially if you were going for an exact replica, but I would work on improving your class naming conventions as well as using proper HTML5 markup semantics. With your sass files I would try to reduce nesting as much as possible, I personally try to not go three levels deep for a selector. There were a few random px values that i didn't understand, but your sizing with rem and % was pretty well done. Also for your box shadows I would generally use the rgba() function to create a black color with a low opacity.

  So yeah most of my advice is just small things and my personal opinion from my experience over the years. The biggest things I'd think about looking is the naming convention that you use for your html elements, as well as ways to reduce nesting in your sass file. I would also think about making parts of your sass rules reusable. @mixin to create common styling and @extend to include fake %classes will help to reduce your style sheet size and the nesting of components.

  2

  ---

  

  Ahmed Alhalfa•40

  @ahmedalhalfaPosted almost 4 years ago

  @steventoben and another thing is that when i use a section element without putting in it a h element i get error in the report here, why ?

  0

  

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  @ahmedalhalfa ah ok I'm sorry I should've been more specific with the section tag. So a section is a container, but basically for text. That's probably why you need an h element inside of the section. A section is kinda like a section in a blog, the h element will be inside the section element and it's basically what that section is about. So it should work with your second section because it contains an h2 element that's basically what the content in that section is about. The first one section can actually just be a div because the content is purely an image. Sorry about that one, I'm trying to help people use the correct HTML tags instead of just using a div for everything in their project, but I should have checked the docs first, so sorry about that!

  2

  

  Ahmed Alhalfa•40

  @ahmedalhalfaPosted almost 4 years ago

  @steventoben no problem steve, thanks for your time and effort that is so appreciated

  0
• 

  Ahmed Alhalfa•40

  @ahmedalhalfaPosted almost 4 years ago

  @steventoben huge thanks to you steve <3, will surely look into your points and improve the project

  0