Ken

Latest comments

• 

  Ken•935

  @kenreibmanSubmitted about 3 years ago

  

  Tip Calculator using CSS Grid, Flexbox, Vanilla JS

  2

  10
  

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @kenreibman 👋🏻

  I looked at your code and I can see what you're missing. You're missing validation, if you take a look at my solution you'll see that you can't input letters, a bunch of zeros, symbols, and invalid value in general. In your case, you're only checking if the input is filled then calculate the number, but you need to consider the edge cases. Other than that, everything looks and works perfectly. Nice job, and sorry for the late response.

  Marked as helpful

  1
• 

  Ken•935

  @kenreibmanSubmitted over 3 years ago

  

  ARTICLE PREVIEW COMPONENT using Flexbox and Vanilla JavaScript

  1

  30
  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @lmaoken 👋🏻

  I took a look at your JS as you asked, and here are some suggestions:

  • First, it would be really great if you declared variables for all the selections, so you don't have to write document.querySelector('') each time. For example:

  const btn = document.querySelector('.btn__share');
  

  of course you should use more descriptive names, this was just a demonstration.

  • Next, I suggest creating classes in CSS and give all the styling that should be applied to the button or whatever you're toggling and then in JS just add and remove those classes or even better use toggle method. You can check out my **solution to see how to do it. When, you give styles with .style it adds inline styling, and then it's pretty much stuck in your HTML, unless you refresh the page, so it would be a lot better to implement those styles in CSS then play with the classes in JS.
  • Lastly, you should add `aria-expanded="false" in HTML, and then according to the state of the pop-up change to true, I also have that implemented in my solution.

  I hope this was helpful 👨🏻‍💻 other than that, you did a superb job, I knew you got this and this is just the begging. You've got a lot to do and to practice, but you'll get there, trust me. Cheers 👾

  Marked as helpful

  1
• 

  Chaman•120

  @chaman-rawatSubmitted over 3 years ago

  

  Four card feature section using CSS Grid

  1

  21
  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @chaman-rawat 👋🏻

  I've got some feedback for the project.

  • For the icons, add aria-hidden="true”, because they're for decoration. You can read more about aria-hidden here. For example:

  <img src="images/icon-team-builder.svg" alt="Team Builder">
  

  • Avoid, positioning items with just margins especially when you're trying to center them. Both Flexbox and Grid have things like align-items, justify-content, etc. you should use them for positioning.
  • Also, avoid fixed units for layouts such as pixels, try to use rems and percentages for things like width.
  • Lastly, to answer your question, there's no way to get accurate screen size from browser DevTools, because sometimes in DevTools things look superb for mobile viewport width, but when you open it on an actual phone some things may not look or work as excepted, but that's pretty rare nowadays.

  I hope this was helpful 👨🏻‍💻 other than that, for the first project you did a great job, well done. Cheers 👾

  Marked as helpful

  2
• 

  Abhijit Sarode•90

  @AbhijitSarodeSubmitted over 3 years ago

  

  Order Summary Component - Using Flex Box

  1

  10
  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @AbhijitSarode 👋🏻

  I've got some suggestions to help you fix the accessibility and HTML issues.

  • In your markup, <div class="card">...</div> should be <main class="card">...</main>. This will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
  • The hero image should have a description in the alt tag, something like, alt="happily dancing girl"
  • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here.
  • If srcset attribute is not used, it should be removed, otherwise it causes some errors. So the image tag should look something like this:

  <img src="/images/icon-music.svg" alt="" aria-hidden="true">
  

  • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.
  • Hero image should have a display: block;, it removes the line underneath the image. If you want to know what's causing it, check out the 3rd section of this video.
  • I won't go into details about resetting CSS, but I'll leave this cool article here, which will make more sense than my brief explanation.
  • Next, I suggest using <h2> or <p> instead of <h4>, because headings in HTML have to increase gradually, such as h1, h2, h3…..
  • li is only allowed in ul as a list item, so you should remove it from <a> tag.

  I hope this was helpful 👨🏻‍💻 all in all, for the second project, you did a good job. Cheers 👾

  Marked as helpful

  1
• 

  CleanCoderK•160

  @CleanCoderKSubmitted over 3 years ago

  

  Order Summary Challenge from Frontend Mentor

  1

  10
  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @green-cyber 👋🏻

  Unfortunately, we cannot see the site. Perhaps, something wrong with the link or the GitHub pages. Please fix the link, so the community can give some feedback.

  I hope this was helpful 👨🏻‍💻 Cheers 👾

  Marked as helpful

  0
• 

  dex1989•20

  @dex1989Submitted over 3 years ago

  

  html5 css write using visual studio

  1

  10
  

  P

  Ken•4,915

  @kens-visualsPosted over 3 years ago

  Hey @dex1989 👋🏻

  I've got some quick tips to help you fix the accessibility and HTML issues.

  • First, I suggest using <h2> or <p> instead of <h4>, because headings in HTML have to increase gradually, such as h1, h2, h3…..
  • Next, this piece of code dex1989</a>. --7>--></html> is causing some HTML issues, just because you didn't put the closing comment tag correctly, so the solution would be to either remove the code or remove the comment tags.
  • Hero image should have a display: block;, it removes the line underneath the image. If you want to know what's causing it, check out the 3rd section of this video.
  • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here. Like this:

  <img src="./images/icon-music.svg" alt="" aria-hidden="true”>
  

  • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.
  • Lastly, I won't go into details about resetting CSS, but I'll leave this cool article here, which will make more sense than my brief explanation.

  I hope this was helpful 👨🏻‍💻 other than that, you did a good job for the first project, nicely done. Cheers 👾

  Marked as helpful

  0