Article preview using TailwindCSS & JS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks bigger but it's fine though the background-color of the overall site should be changed to what the design is using. The site is responsive and the mobile state looks great.

  Some other suggestions would be:

  • Always have a main element to wrap the main content of your page. On this one, the .gridLayout should be using the main instead of div.
  • Using margin to center on all axis an element is not consistent, I normally suggest not using it but on this one, by removing it, the layout's size changes since there is no definition of max-width on the .gridLayout or in the container inside it.
  • The left-side image could use a more descriptive alt since drawers is too ambiguous right.
  • I wouldn't really use heading tag on the bold text since it is just like an opening text of a blog rather than being a title of something.
  • Also, an h1 is always required on a page, since there is no visible text that could be h1 , it would be a screen-reader only h1. Meaning this will be hidden for sighted users and only be visible for screen-reader users, search about sr-only stylings and see how it is used. The h1 text should describe what is the main content is all about, this h1 would be placed as the first text-content inside the main element. Have a look at this simple snippet of mine about that method I mentioned comments are already in there but if you have more queries, just let me know.
  • The person's name as well the text below it should not be inside a ul since those are not really list of items. Use a div to contain both, then the person's name should be a heading tag but use p tag on the text below it. You use heading on the person's name since they are the focal point of the component.
  • You should have used button on the toggle. Remember that interactive components needs to use interactive elements. By using img on it, you are not making it accessible, only mouse events can toggle it but not by other peripherals.
  • The button toggle for the social-media should have aria-expanded="false" as a default attribute and it will be set to true if the user toggles it and vice-versa. It also should have aria-label attribute on it, defining what the button will do, it could be like aria-label="share on social media dropdown". To provide clear example, have look at Kasia's solution on this challenge,
  • Each a tag that wraps social media, it should have either aria-label attribute or screen-reader element inside it. The value for whatever method you will use should be the name of the social media like aria-label="facebook" on the facebook link a tag. This way, users will know where this link would take them.
  • Lastly, there are lots of classes going on around here, are you using css framework? If yes, I suggest not using right now if you are starting out in frontend, use only native css or scss to build up that fundamentals properly.

  Still, great job again on this one.

  0