Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions0Comments7

All comments

  • Guliye• 290

    @guliye91

    Submitted

    HI Coders! I just finsihed 'FAQ accordion Card'. I used Javascript. DONT BOTHER THE DESIGN. I just wanted to practice my JS knowledge.

    kindly gi through and give me feedback on issues of js functionality in js.

    Responsive FAQ accordion using JS

    1

    Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    Hi Guliye

    I noticed a few things about your Javascript:

    • I'd use functions to make clear what is happening. You could make a function called resetAccordion() and a function called showAccordion().
    • I'd make a bit more use of comments.
    • The accordion can only be opened by clicking the icon, but it'd be more user-friendly to have it open upon clicking .question.

    Have a nice day. ✌

    2
  • Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    Hey Rodrigo,

    I've taken a quick look and I noticed the following:

    • Instead of width: 375px I'd use width: 100%; max-width: 375px as this helps with responsive.
    • The blue background pattern images don't show up on mobile.
    • I'd add a margin to the sides of the page on mobile, so that the card doesn't touch the edges. Something like 5px to 15px should be perfect.
    • You could use the BEM method for your CSS. Not really necessary here as it's pretty clear, but I'd look into it nonetheless.

    Your code looks clean. Have a nice day. 😊

    3
  • Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    Hey Bryan

    I've taken a look and I noticed the following things:

    • Your responsive design (376px and up) has some flaws.
    • You tend to use > in your CSS. I would make the elements specific by utilizing the BEM method.
    • You're using CSS variables -- that's great.

    Happy coding 😄

    0
  • Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    Hi Karim

    I took a quick look and I have some suggestions:

    • I'd stop using <br> to create space between elements. Instead, use margin or padding.
    • Your indentation could be improved -- perhaps look at how a code beautifier would indent your code.
    • In some cases I see inline-styling (such as <h1 style="font-size: 65px">2.7m+</h1>. I would put all styling in your CSS file instead.
    • Your class names and id's could be more descriptive. It's difficult to immediately tell what #final or #final2 is.
    • You also have some html and accessibility issues (there's a report of it on this page).

    You're on the right track. 😄

    2
  • Vanessa Martin• 155

    @vsm1996

    Submitted

    Woohoo! Another solution complete! 😄 This one was a bit intimidating, but once I sunk my teeth in, it was a blast! 😍 Please feel free to give me any suggestions, as I had difficulty with placement. I also feel like some of my logic could be a lot cleaner.

    Three things I plan on implementing are:

    1. making this a pwa (mostly for my own enjoyment)
    2. adding transitions for the disappearing quote/appearing extra details
    3. Initial loading screen to give APIs time to send data

    Happy Coding! ✨🍃

    Clock App Using React + CSS (Flexbox, Web-First Solution)

    3

    Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    Looks great! I love that you put the CSS files with the corresponding component -- before this I'd have it under ./src/css.

    The only thing I'd change is I'd use functional components.

    1
  • Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    I love your usage of variables in CSS. I also like that you've used prefixes where needed.

    Some things I'd do differently are:

    • Change width: x% to width: 100%; max-width: x%, as this helps with responsive design.
    • I'd limit the amount of classes I put on the html elements, because it becomes harder to read otherwise (though I understand that with bootstrap it's inevitable to add classes). I'd look into the BEM method.
    • I noticed you need to use margin-top: -340px; margin-right: -340px; on a class, which points to an issue in a part of the CSS.

    Overall it looks clean and structured. :)

    2
  • Mike Schmidt• 90

    @Schmidt217

    Submitted

    I'd love to get some feedback on my code if anyone has any!

    HTML/ CSS

    1

    Amir Jacobs• 165

    @Amir-Jacobs

    Posted

    I love the fact that you used "preconnect" on the google font.

    There's are a few things I'd do differently:

    • I'd avoid using position: relative and position: absolute if I can.
    • I would change width: x% to width: 100%; max-width: x% as this helps with responsive design ("x" being the width you want).
    • something is making your body too tall -- it probably has something to do with your use of position, so I'd use overflow: hidden on the body to make it not scrollable.
    • I would put the stylesheet <link> elements after the title and the script elements at the bottom of your <body> element.
    • I'd remove the .attribute styling and move it to your stylesheet.
    • I'd add a subtle transition to your buttons when hovering by using transition: 0.3s ease all.
    • I'd put the <img> element in the <div class="container"> too.

    You're doing amazing so far. Keep it on! :)

    1