Article Preview Comp. with BEM, SCSS and Vanilla JS

Community feedback

• 

  Raymart Pamplona•16,060

  @pikapikamartPosted over 3 years ago

  Hey, great work on this one. First, it is great that you managed to pull this off, awesome. The layout in desktop looks great, responsive and the mobile layout looks great as well.

  Some suggestions would be:

  • Avoid using height: 100vh on a large container especially the body tag, this makes the element's height limited to the remaining viewport/screen's height. Instead use min-height: 100vh, this takes full height and allows the element to expand if it needs to. Also you don't need the width: 100% as well.
  • If you inspect the .container selector, you will see that it has no dimension, it is because the figure inside it is using position: absolute which makes the element out of the flow. You don't need that in the element. You can remove the position: absolute and also, you don't need this on the figure:

  position: absolute;
  position: relative;
  top: 50%; 
  left: 50%; 
   transform: translate(-50%, -50%);
  

  • I think the highlighted text is not really heading tag, since most likely on a testimonial, the person's name should be the heading tag because testimonial doesn't really have a topic title. Instead, I would make use of the h1 in here as a screen-reader only text, meaning it will use an sr-only class, you can search that one up and how it is used.
  • alt value for the person's image should only be the name of the person alt="Michelle Appleton". Avoid adding words that relates to "graphic" such as "avatar, image, logo.." since it is already an image, no need to describe it as one.
  • Name of the person should be a heading tag.
  • The toggle works but it is not accessible for all. When creating interactive components, use interactive elements. On this one, you should use button and not span. The button will have aria-expanded="false" as a default attribute. This will be change to true if the button is toggled. Since there is no text content inside the button that a screen-reader can read, you would use an aria-label attribute or an sr-only element inside the button. The value for whatever method you will use should describe what does the button does. A value could be "social media platform to share info".
  • The svg inside the toggle needs to be hidden since it is just a decoration, so you should add aria-hidden="true" as an attribute on the svg.
  • The placement of the toggle and the dropdown is incorrect. The dropdown should be placed after the toggle on your markup, swap them up.
  • Since the social media links are "list" of links, you could use an ul element on them.
  • The aria-label for each a tag should be the name of the social media, like alt="facebook" this way, users will know where this link would take them.
  • The svg inside the a tag should be hidden, use aria-hidden="true" on it.

  Aside from those, great work again on this one.

  Marked as helpful

  0
• 

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @pikamart 👋🏻

  Thanks for the feedback, I'll follow up.

  position: absolute;
  position: relative;
  top: 50%; 
  left: 50%; 
   transform: translate(-50%, -50%);
  

  Also thanks for pointing this out, it was a leftover code from an experiment. Cheers 👾

  1
• 

  Ken•935

  @kenreibmanPosted over 3 years ago

  Hey, I've been stuck on this challenge for a week now - trying to teach myself basic JS to make that sharing preview work.

  I've been watching Vanilla JS crash courses, going through the freecodecamp course for JS, but it doesn't seem to explain how I would use JS to do what this project is asking.

  Do you have any recommendations or suggestions on how I could teach myself? I don't want to copy someone else's solution without having a thorough understanding.

  0

  ---

  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @lmaoken, my suggestion would be to not rush with JS, if you haven't taken a full-blown course where every little piece is explained in details please do so. If you want any recommendations, here's one, it's on Udemy, and they do have some sales going on occasionally. Don't pay the full price, you can get the entire course for like $18 instead of $200. JS is one of those things that you won't learn with a crash course or a couple of articles, if you want to learn it fundamentally, of course. You can review it with a crash course or use those articles as additional resources, but in my opinion not with a crash course. And for this type of stuff, you'd want to watch some DOM operations. Again, I'm not really familiar with your JS skills and I don't want to assume anything, but this is my suggestion. Get a proper course go through everything and practice every time you learn something new, I'm suggesting this path of learning because JS is a fundamental piece of web development. If you know JS well, you'll easily learn any framework/library, and you'll easily learn node JS. Lastly, yes, never copy anyone's code just for the sake of the completion of the challenge or anything else, unless you fully understand how it works. Because, we, programmers are guilty of copying some code from a Google search result just to make our code work 😅

  I hope this was helpful 👨🏻‍💻 Cheers 👾

  1

  

  Ken•935

  @kenreibmanPosted over 3 years ago

  @kens-visuals Thank you! I know this was 14 days ago but for the last 14 days I have been teaching myself non-stop. I seem to have grasped the basics and finally was able to complete this on my own! If you could take a look, I would really appreciate it. I believe my js isn't the cleanest, but any feedback on the JavaScript would be amazing!

  0