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

All comments

  • Ken•4,915
    @kens-visualsPosted over 2 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 about 3 years ago

    Wow, this took so long to make. After countless hours of studying JavaScript, watching crash courses and tutorials. I was finally able to create a simple pop-up on a page.

    I'm very confident in HTML and CSS now, especially Flexbox. I hope I can have challenges that push me to utilize more CSS Grid. I also appreciate the challenge and countless hours of studying I went through to learn a little JavaScript.

    I am still in the learning process of vanilla JavaScript, and I know the code I wrote for the pop-up is not exactly the cleanest. If anyone has feedback on how I can make the code cleaner, or acceptable for the industry, I would greatly appreciate any sort of feedback that you give me out of your time.

    ARTICLE PREVIEW COMPONENT using Flexbox and Vanilla JavaScript

    1
    Ken•4,915
    @kens-visualsPosted about 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 about 3 years ago

    When I was using Firefox and Chrome "Responsive design mode" I do not get the accurate device width. It was slightly larger than the actual pixel width I set. So is there any method or workaround to get accurate screen size?

    Four card feature section using CSS Grid

    1
    Ken•4,915
    @kens-visualsPosted about 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
  • Ken•4,915
    @kens-visualsPosted about 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
  • Ken•4,915
    @kens-visualsPosted about 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 about 3 years ago

    that is my first proect, i just stat to code for the project i use html5 and css i have use visual studio for write the code. Every comment and advice is welcome to improve myself.

    html5 css write using visual studio

    1
    Ken•4,915
    @kens-visualsPosted about 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
  • Ken•4,915
    @kens-visualsPosted about 3 years ago

    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 👾

    0
  • Ctrl+FJ•810
    @FlorianJourdeSubmitted about 3 years ago

    So, my main question would be : how to use container by the same way bootstrap do ? I mean, I definided some width to my page to be as close as the design at possible, but I guess it would be better if my colors continue on right and left, instead of this black background. How can I define a nice wrapper in native HTML/CSS ?

    Also, I wanted to animate my toggle burger-menu, but I struggled because of display: block... any suggestions ?

    However, pretty interesting and real-sized exercice ! Liked it, altrough it was a bit long !

    Sunnyside agency landing page with HTML & CSS

    1
    Ken•4,915
    @kens-visualsPosted about 3 years ago

    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
    1
  • Ryan Flores•60
    @RyanFloresTTSubmitted about 3 years ago

    Any feedback is greatly appreciated! I only worried about the Desktop Version, going to learn more to see what I can do about the mobile version.

    3 Column Preview Card Component

    1
    Ken•4,915
    @kens-visualsPosted about 3 years ago

    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
    2
  • Orincy Whyte•20
    @oswhytecodesSubmitted about 3 years ago

    Hi all This is my first challenge, and I think I did pretty decent. all that aside, I can't figure out how to perfectly fit the main image inside the div. You can even see I haven't been able to show the top part of the container either. If anyone can help, I am ready for the feedback. Thank you.

    Order Summary Card test1

    2
    Ken•4,915
    @kens-visualsPosted about 3 years ago

    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 👾

    1
  • Ken•4,915
    @kens-visualsPosted about 3 years ago

    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
    0
  • Meri•10
    @merithekidSubmitted about 3 years ago

    I've only just started doing projects. How can i clean up my code and any practices you would recommend?

    3-column preview card component

    1
    Ken•4,915
    @kens-visualsPosted about 3 years ago

    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 👾

    0