Skip to content
OverviewSolutions
10
Comments
10
P

David Lemvigh

@dlemvighCopenhagen200 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • P
    David Lemvigh•200
    @dlemvigh

    The visual

    • There is a scrollbar near the faces
    • The purple buttons are a bit brighter in color that design

    Generally really good solution, nice and responsive on all screen sizes.

    The code

    • Nice semantic html
    • Well organized css classes
    • An elegant solution to the desktop faces. The design shows the faces closer to the edges. But with your solution that could easily be changed by adding justify-content: space-between to the .banner__container. (I also like your design better than the figma design on that front)
    • I think the cause of the scrollbar is the overflow-x: hidden, which I don't understand the reason for. Removing it removes the scrollbar. (It is not the first time I've see overflow-x/overflow-y having unexpected interactions, but I don't know/remember why)

    Note: I tested in Chrome on Windows 11, if that helps with debugging the scrollbar.

  • P
    Ishmael Sunday•250
    @okayishmael
    What are you most proud of, and what would you do differently next time?

    Mixing grid and flexbox to achieve my goal.

    What challenges did you encounter, and how did you overcome them?

    The design indicated 13px font size.16px/ 1rem is what I used for readability. I some people can see to well. I am just advocating for users

    What specific areas of your project would you like help with?

    My is seems a little bigger.

    Responsive testimonials grid section | htm5, css3

    1
    P
    David Lemvigh•200
    @dlemvigh

    The Visuals

    • Looks really good
    • Avatar outlines a bit different than design, but looks good
    • Nice 4, 3, and 1 column layouts
    • A bit extra body margin on mobile layout compared to design, making the content very squezed on when get below 400px
    • Missing "watermark" on purple card (also found it annoying to add myself without getting it on top of the text, but find a nice way by setting background-image on the card).

    Visually really good, only small notes

    The Code

    • The is an attempt at css grid, but then a lot of flexbox inside doing most of the work (grid-template-columns: repeat(6); - repeat is missing its second parameter so gets ignored)
    • A bit of grid and flexbox nesting, would have been nice to have been done with a single grid. Maybe using order to put the "kira" card on the bottom.
    • Well structured html
    • Nice use of css variables

    Flexbox usage really good. Grid usage could have been better. But in general really nice solution

  • P
    Will Cooley•190
    @CooleyWC
    What are you most proud of, and what would you do differently next time?

    This was my first time using Sass for CSS

    What specific areas of your project would you like help with?

    I know my box-shadow for the cards isn't quite like the Figma design - I could use help with this.

    Four Card Feature

    2
    P
    David Lemvigh•200
    @dlemvigh

    The visual

    Looks great, minor discrepancies from the design, but it feels more like a stylistic choises than mistakes.

    Nice use of CSS grid

    The code

    Nice clean HTML, the only petty comment is that you have multiple h1 headings which might not be the best for accessibility, but I could be wrong on that.

    The scss files are generally well structured. There is a bit of .hero styling that has snuck into _typography.scss which is confusion, and probably should be in _hero.scss.

    Marked as helpful
  • P
    David Lemvigh•200
    @dlemvigh

    ** The visuals **

    • many visual differences from the design ** background color ** fonts ** image size
    • the layout kinda work on exactly the breakpoints 320px, but looks pretty broken on most other width < 460px

    ** The code **

    • A lot of inline-styles
    • Mix of classes and inline-styles
    • Does use flexbox and media queries for layout
  • P
    David Lemvigh•200
    @dlemvigh

    Visual

    The good

    • Looks great, really well done.
    • Love that you have added a loading state that wasn't in the design.
    • Audio playback worked really well

    The bad

    • font switcher is a bit off, options are light in dark-mode and dark in light-mode
    • keyboard accessibility are missing a few things ** dark mode can be toggled, but there is no focus outline ** font switcher cannot be accessed using keyboard navigation (tabbing throught the page). *** could potentially be fixed by adding tab-index=0, focus styling, and keydown event handler

    Code

    • 400 lines of react in one file is a lot, would be better if it was split up into at least a couple of separate files
    • looks like it is a create-react-app which has been deprecated and not updated since 2022

    CSS

    The good

    • I like the of variables and the mixings
    • especially the $typography-styles, letting you mimic the figma usage of defined typographies

    The bad

    • 700 lines of css in a single files is a lot.
    • a lot of nesting in the rules. This causes increased specificity which can make extending the classes in the future difficult, without !important and other hacks.
    • I'll be honest I got overwhealmed by everything being in one big filed, and haven't reviewed it that well.
  • P
    David Lemvigh•200
    @dlemvigh

    the good

    • I am impressed at that the page is implemented in vanilla js
    • well organized html (minor mismatch in label for mixing kebab-case and camelCase)

    the bad

    • focus styling a bit off for year and interest rate, looks different than the focus styling for amount
    • error styling a bit off. The input addon ($ , year , %) does not have red background.
    • radio inputs have no focus styling, so keyboard users have no outline to show focused element.
    • no validation messages are shown ("this field is required")
    • clear doesn't clear validation state. When clicking clear all, I would have expected the validate state to also be cleared.
    • "clear all" not keyboard accessible (could be fixed with tabIndex="0") or making it a button with styling

    conclusion

    A solution that fulfills the technical requirements, with semantic HTML. There are many deviations from the design in terms of styling.

    Marked as helpful
View more comments