Skip to content
OverviewSolutions
43
Comments
298
P
Ken
@kens-visuals

All comments

  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    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
  • P
    Ken•4,915
    @kens-visuals

    Hey @muhabibta 👋🏻

    I have some suggestions to help you fix the accessibility issues and some other things.

    • In your markup, <div class="container">...</div> should be <main class="container">...</main> and this will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
    • Next, I suggest using <h2> or <p> instead of <h4>, because headings in HTML have to increase gradually, such as h1, h2, h3…..
    • Lastly, let's bring the card to the center of the page, and here's how to do it:
    body {
    font-size: 16px;
    font-family: "Karla", sans-serif;
    background-color: var(--Light-gray);
    min-height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
}

    and once you do this, you can remove margin: 8% auto; from .container.

    I hope this was helpful 👨🏻‍💻 overall, you did a nice job, keep it up. Cheers 👾

  • P
    Ken•4,915
    @kens-visuals

    Hey @FlorianJourde 👋🏻

    I have some suggestions for the project.

    • I recommend using ul and li for the both navs at the top and bottom, also you could put the social media icons in a list as well. Although they don't look like so, but they are lists and it would be semantically more correct to write them like so.
    • Next, -for the logo, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here.
    • alt tags for the user's images should be their names, like this:
    <img src="images/image-emily.jpg" alt="Emily R.">
    • Lastly, to answer your question about width, avoid using fixed widths that's what causing problem here and add those black lines on the both sides on every viewport width. For such big things like layouts, images, etc. the best choice would relative units, such as percentages.

    I hope this was helpful 👨🏻‍💻 other than that, you did a pixel perfect job, well done. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @RyanFloresTT 👋🏻

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

    • In your markup, <div class="row">...</div> should be <main class="row">...</main> and <div class="attribution">...</div> should be <footer class="attribution">...</footer>. These will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
    • For the car icons, add aria-hidden="true”, because they for decoration. You can read more about aria-hidden here. Also, I suggest putting them in <img> tags, so all together will look like this:
    <img src="./images/icon-luxury.svg" alt="" aria-hidden="true”>
    • Also, I suggest implementing :hover state, which you can find in design folder active-state image, after you implement it you can also add 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 👨🏻‍💻 If you want to get better at responsive websites, learn mobile first workflow, there are a thousand of articles about it. All in all you did a nice job, for the second job. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @oswhyteknits 👋🏻

    I have some suggestions to help you fix the accessibility issues and some other things.

    • In your markup, <div class="container">...</div> should be <main class="container">...</main> and <div class="attribution">...</div> should be <footer class="attribution">...</footer>. These will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
    • 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.
    • Next, I suggest using <h2> or <p> instead of <h5>, because headings in HTML have to increase gradually, such as h1, h2, h3….
    • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here. To illustrate:
    <img src="images/icon-music.svg" alt="" aria-hidden="true”>
    • In order to fix the border-radius just add overflow: hidden; to .container.
    • Also, I suggest implementing :hover state, which you can find in design folder active-state image, after you implement it you can also add transition: all 0.2s; to the button and the links, this will make :hover smoother.
    • Lastly, let's bring the card to the center of the screen, so I made a couple of changes in body:
    body {
    background-image: url(images/pattern-background-desktop.svg);
    font-family: Helvetica, sans-serif;
    font-size: 16px;
    text-align: center;
    margin: 0;
    width: 100%;
    background-repeat: no-repeat;
    background-size: contain;
    min-height: 100vh;
    display: flex;
    flex-direction: column;
    align-items: center;
    justify-content: center;
    background-color: #e0e8ff;
}

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

  • P
    Ken•4,915
    @kens-visuals

    Hey @carlosedugoc 👋🏻

    I have some suggestions to help you fix the accessibility issues and some other things.

    • In your markup, <div class="container">...</div> should be <main class="container">...</main> and this will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
    • Next, make sure to add alt text, in this case it should look like this:
    <img src="./images/image-jeremy.png" alt="Jeremy Robson">
    • 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.
    • You've named the classes good enough, but if you're trying to boost productivity and improve CSS, I'd suggest learning BEM convention.
    • Also, I suggest implementing :hover state on ellipsis, which you can find in design folder active-state image, after you implement it you can also add transition: all 0.2s; to the boxes and ellipsis, this will make :hover smoother.

    I hope this was helpful 👨🏻‍💻 other than that, you did a great job, keep it up. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @merithekid 👋🏻

    Unfortunately, we cannot see the project, perhaps due to a broken link. Please fix the issue, so the community can give some feedback.

    I hope this was helpful 👨🏻‍💻 Cheers 👾

  • P
    Ken•4,915
    @kens-visuals

    Hey @richardtr123 👋🏻

    I have some suggestions to help you fix the accessibility issues and some other things.

    • 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.
    • 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, the footer should be at the bottom of the page.
    • The text in the bottom right block should be in ul li because it's a list.

    I hope this was helpful 👨🏻‍💻 other than that, you did a nice job, especially with responsiveness, nicely done. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @smccourtb 👋🏻

    I have some suggestions to help you fix the accessibility and HTML issues.

    • In your markup, <section class="container">...</section> should be <main class="container">...</main>.
    • For the car icons, add aria-hidden="true”, because they're for decoration. You can read more about aria-hidden here. These will fix the accessibility and HTML issues. Don't forget to generate a new repot once you fix the issues. To illustrate:
    <img src="./images/icon-luxury.svg" alt="" aria-hidden="true”>
    • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother. And in :hover add cursor: pointer; for the finger cursor.
    • 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.
    • Lastly, let's bring the card to the center of the screen with flexbox, because using position: absolute to position such big stuff can be tricky and may have some problems on mobile viewport width, and here's how to do it:
    body {
    display: flex;
    align-items: center;
    font-size: 15px;
    font-family: "Lexend Deca", sans-serif;
    padding: 0px;
    margin: 0px;
    min-height: 100vh;
    justify-content: center;
}

.container {
    display: flex;
    width: 38.5rem;
    color: hsla(0, 0%, 100%, 0.75);
}

    I hope this was helpful 👨🏻‍💻 other than that, you did a good job, keep it up. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @DummyKen 👋🏻 Ken's here 😅

    I have some suggestions to help you fix the accessibility and HTML issues.

    • In your markup, <section class="card">...</section> should be <main class="">...</main> and <div class="attribution">...</div> should be <footer class="attribution">...</footer>. These will fix the accessibility and HTML issues. Don't forget to generate a new repot once you fix the issues.
    • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here. To illustrate:
    <img src="./images/icon-sedans.svg" alt="" aria-hidden="true”>
    • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.
    • I've made a couple of changes in .card, to fix border-radius and some other things, so here it is:
    .card {
    display: flex;
    flex-direction: row;
    width: 700px;
    border-radius: 10px;
    overflow: hidden;
}

    as you can see, I omitted height because fixed height prevents your website from being fully responsive. Also, I omitted margin because I have a different suggestion to help you fix the card in the center.

    • Now let's bring the card to the center of the scree:
    body {
    background: hsl(0, 0%, 95%);
    min-height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
    flex-direction: column;
}

    I just used flexbox, which will make sure that on any viewport width, the card is positioned correctly.

    • 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 great job for the first project, well done. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @Theguydev 👋🏻

    I have some suggestions to help you fix the accessibility issues and some other things.

    • In your markup, <div class="container">...</div> should be <main class="container">...</main> and <div class="header-container">...</div> should be <header class="header-container">...</header>. These will fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
    • For the icons, add aria-hidden="true”, because they are for decoration. You can read more about aria-hidden here. For example:
    <img src="assets/icon-supervisor.svg" alt="" aria-hidden="true”>
    • Next, <p class="main">...</p> should have text-align: center;, especially on mobile viewport width.
    • I won't go into many 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 👨🏻‍💻 all in all, you did a good job with the project, keep it up. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @amberensley 👋🏻

    I have some feedback for the project.

    • For the music icon, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here.
    • 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
    • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.
    • I won't go into many details about resetting CSS, but I'll leave this cool article here, which will make more sense than my brief explanation.
    • Lastly, let's bring the card to the center of the screen, just add min-height: 100vh to the body, and you're good to go.

    I hope this was helpful 👨🏻‍💻 to answer to your question, I'd say in my opinion it's good to use relative units for the most of the stuff, because fixed units are not good if you're trying to build a responsive website or even a small component. Other than that, you did a great job with your first project, well done. Cheers 👾

    Marked as helpful
  • P
    Ken•4,915
    @kens-visuals

    Hey @QZheng16 👋🏻

    I have some suggestions to help you fix the HTML issues and some other things.

    • First, instead of img it should be picture in this case, since you're using responsive images. Also, add aria-hidden="true”, because it's for decoration. You can read more about aria-hidden here. So all of this should look like this:
    <picture>
    <source srcset="images/image-header-mobile.jpg 400w" 
            media="(max-width: 400px)">
    <img src="images/image-header-desktop.jpg" alt="" aria-hidden="true”/>
</picture>
    • I won't go into many details about resetting CSS, but I'll leave this cool article here, which will make more sense than my brief explanation.
    • To answer your question, in modern day and age CSS allows us to make some responsive layouts without using media queries a lot of the time, but it's not always true. So even though flexbox and grid are here to help us, yet they cannot replace media queries. Because media queries are not only used for layout stuff, but also font-size margin, padding, etc.

    I hope this was helpful 👨🏻‍💻 other than that, you did a nice job for your second project, keep it up. Cheers 👾

    Marked as helpful