Skip to content
OverviewSolutions
16
Comments
10
JK
@JakubKepak

All comments

  • JK•345
    @JakubKepak

    Very nice job Radomir! I've noticed a small thing which might take your solution even higher, in my opinion :).

    Sometimes you use under score in variables names. For instance, up_vote in In ButtonUpVote.js. I would stick with camelCase only, it's more consistent.

    Cheers!

  • JK•345
    @JakubKepak

    I like your solution! very nice 👏

  • JK•345
    @JakubKepak

    Hi Mohak, your solution is awesome. 👏👌

    I would maybe add max-width to keep it nice on larger screens.

    Keep it up! Rally like it! 🙌

  • JK•345
    @JakubKepak

    Hi Louis, very nice work! 👌👏

    In your code, you use px for size. I would maybe use some relative measure like rem or em. Also, I would fill in alt attributes for accessibility as the report suggests.

    Anyway, I really like your solution. Keep it up! 🙌

  • JK•345
    @JakubKepak

    Hi Chetan! very nice solution! 👏

    I would maybe consider adding media query on about 375px and reorder the columns.

    Anyway, good job!

  • JK•345
    @JakubKepak

    Hi Chris-Ann C. ! Nice work 👏

    To centre all content to the centre of a page a would set display:flex to the body and then add justify-content: centre and align-items: centre. In order to work, you would also need to set the property height on the body element. For instance height:100vh. Also, I would maybe set max-width proepty to .wrapper in order not to fill all space in wide screens.

    Anyway, nice job! Keep improving 🙌

  • JK•345
    @JakubKepak

    Hi Lpz, nice job! you have it almost pixel perfect! 👌👏

    I would maybe change position of the image with object-fit property be more like the design (https://css-tricks.com/almanac/properties/o/object-fit/)

    anyway, nice work! Keep improving! 🙌

  • JK•345
    @JakubKepak

    Hi InduRajput, nice work! The desktop view is pretty close to the design. 👏

    My food for thoughts:

    • I would name class attributes in HTML something meaningful. That is just my opinion. It's easier to navigate in css file
    • I would position the boxes ( sectionX in your code) with css grid or flexbox. I would create a container for all with display: flex or grid. You can then shift them by adding margin.
    • Similarly, I would approach the rating-sec container.

    With this you can then change ordering by changing display property on container to flex-direction: column, if you would use flexbox.

    Keep coding! 🙌

  • JK•345
    @JakubKepak

    Hi Michelle, nice work! I would maybe add media query In about 1000px and perhaps change the layout to two columns or smaller the font-size.

  • JK•345
    @JakubKepak

    Man, that is one awesome solution! One thing I would maybe do differently is to set outline to none when .person_article-share is focused and I would add .person_article-share:focus same as you have .person_article-share:hover.

    But that's maybe just my view on outlines.

    Really nice job!