Skip to content
OverviewSolutions
0
Comments
7
Amir Jacobs
@Amir-Jacobs

All comments

  • Guliye•290
    @guliye91Submitted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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
    @vsm1996Submitted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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-JacobsPosted almost 4 years ago

    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
    @Schmidt217Submitted almost 4 years ago

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

    HTML/ CSS

    1
    Amir Jacobs•165
    @Amir-JacobsPosted almost 4 years ago

    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