Skip to content
OverviewSolutions
21
Comments
9
Gediminas Makutenas
@Senatrius

All comments

  • 0xev•120
    @0xevSubmitted almost 2 years ago

    My first "project" using display: grid

    Did I create columns right or is there a different / better approach?

    Since there is 1 row and 3 columns

      display: grid;
  grid-template-columns: 1fr 1fr 1fr;

    3-Column-Preview-Card-Component

    1
    Gediminas Makutenas•765
    @SenatriusPosted almost 2 years ago

    Hi there! Nice work on the project. Yes, in cases like this, grid is a good option. Got a couple suggestions below.

    • Grid template shorthand: a better way to write it when all columns are the same width is grid-template-columns: repeat(3, 1fr) which is the same as 1fr 1fr 1fr but now if you want to change 1fr to let's say 15rem, you only have to change it once, not 3 times. Not necessary here but good for the future.

    • Missing media queries: it seems you are missing media queries for smaller screens. Add @media screen and (max-width: [whatever width you want to change the styles at]) {} and add these two lines grid-template-rows: repeat(3, 1fr); grid-template-columns: 1fr

    • Responsive width: replacing width: 42rem with max-width: 42rem in the .card class will make the component responsive and shrink if it doesn't fit on the screen.

    Besides that, things look good to me. Keep it up :)

    Marked as helpful
    0
  • Secre•210
    @SecreSwalowtailSubmitted almost 2 years ago

    I can't figure out how to change the background color of the image and add the view icon on hover

    Responsive NFT preview card

    1
    Gediminas Makutenas•765
    @SenatriusPosted almost 2 years ago

    Nice work on this project! Only a few things to fix.

    • Overflow: there's no need to have the overflow: hidden on the body tag. With it, if you resize the window so it's smaller than the card, it just goes off screen with no way to see it. It's better to remove the overflow: hidden and have the scroll bars if they're needed, you never know what device a user views it on.

    • Unnecessary nesting: maybe I can't tell in the dev tools, but I don't think div.wrapper around the div.card does anything and can be removed. Ignore if I'm wrong here.

    • Headings: Use h1 instead of h2 and style the font size if needed. Never use heading tags for styling purposes.

    • Overlay on hover: to answer the question you asked about changing color on hover, you could use a separate absolute position div and style it based on whether the image container is being hovered or not. It would look something like this

    <div class="image">
    <div class="overlay">[add your icon or whatever you need here to show up on hover]</div>
    <img src="images/image-equilibrium.jpg" class="card__image" alt="product_image">
</div>

.image {
    position: relative;
}

.overlay {
    display: none;
    position: absolute;
    inset: 0;
    background: [color you want]
}

.image:hover > .overlay {
    display: block;
}

    you can style the div however you like. or use some other tag if you want to it be clickable, it should work the same.

    Keep it up :)

    0
  • Simon•10
    @SimonBeneckeSubmitted almost 2 years ago

    This was my first challenge attempt and my first project other than following along with guides. I read that the best way to go about it is to do the HTML section first and then style with css afterwards, however I did find that there are times where I go back to the html to either add or remove sections. I have no specific questions. Any feedback is welcomed.

    QC Code Completed challenge

    2
    Gediminas Makutenas•765
    @SenatriusPosted almost 2 years ago

    Great job on your first unguided project :) There are just a few things that you could improve to make it even better.

    • Card padding: instead of manually setting the image width using pixels, set image width to '100%', height to 'auto' and remove the top margin. Then add padding: 1rem and box-sizing: border-box to the card. In this case, visually nothing changes but for the future, if you ever want to increase or decrease the gap between card edge and image, you'll only have to change a single value.

    • Responsive units: I see you're using pixels everywhere. I'd suggest switching them out for rem units so the component adjusts to the users browser settings. Some people need to set a higher default font size in the browser due to disability or other reasons and using pixels prevents that. Read up a bit more on responsive units here

    • Semantic HTML: in the same vein as responsive units, semantic html helps people with disabilities who use screen readers to browse your pages easier. In this case, change div.white-background to main.white-background, div.text-in-white to section.text-in-white, and div.attribution to footer.attribution. Can read up more on semantic HTML here.. Also, here's a good reference for semantic HTML tags, very useful when learning.

    Outside of these few things, it looks good. Keep it up :)

    Marked as helpful
    1
  • Gediminas Makutenas•765
    @SenatriusPosted almost 2 years ago

    Not bad, it looks pretty good. There are a couple changes you could make to make it even better.

    • The circle in the todo input field: Instead of trying to eyeball the center using margins, you could add display: flex; align-items: center; to the div wrapping the circle, similar to how you centered the button and span elements in the todo rows.

    • Checkboxes instead of buttons: After all, it is a checklist of items and that's what checkboxes are best used for. And it comes with a lot of accessibility already built in that you don't have to add yourself when using regular buttons. This is generally what it would look like...

    <div class="todo item">
    <input id="random id" type="checkbox">
    <label for="random id">This is a todo item</label>
</div>

    Of course this code is very simplified and you'd have to adjust it to fit your own code. I recommend checking this blog post out to see how to style a custom checkbox to match the design. It's not the exact same use case, but the concepts are similar enough. https://blog.logrocket.com/building-custom-checkbox-react/

    Outside of that, it seems good to me :) Keep it up.

    Marked as helpful
    1
  • Gediminas Makutenas•765
    @SenatriusPosted over 4 years ago

    Hi there.

    Not gonna lie, gradient borders intrigued me. Intrigued me enough to do some research and find out that gradient borders aren't a thing :( I did find a different way to add a sort of "border".

    As for the shadow making it look deep, it's a box shadow with an "inset" property.

    I've played around with both, left some comments so you can make more sense of it than I did. https://codepen.io/Senatrius/pen/PoqdNae

    Keep it up!

    1
  • Akam Foad•245
    @akamfoadSubmitted over 4 years ago

    I really love those challenges, and doing them made me comfortable and confidence about my skills that are growing. Also I have a question, when I done challenges I usually use accessibility tool in Firefox it shows me some accessibility issues, some are I made it, but some of them related to the design required in the challenge, I want your words on this if you interested or have same issue, thanks♥.

    URL shortening API landing page

    1
    Gediminas Makutenas•765
    @SenatriusPosted over 4 years ago

    Hello there.

    The page looks pretty good, seems to be responsive on all screens and the code (at least parts of code I can understand, haven't learned React quite yet) seems clean and easy to read.

    If there was one thing though, the navigation links in the header are the wrong color. Also the buttons and input fields could probably do with a little bit of extra padding. Other than that, this seems good to me :)

    As for the accessibility, I personally just leave the way it is in the design preview. If the color contrast is off, then it's off, if the form doesn't have a label then I guess it won't have a label. It breaks some guidelines, but I'm sure the UI designers wouldn't appreciate developers changing up their design in the real workplace either, so it is what it is :)

    Keep up the good work!

    2
  • Gediminas Makutenas•765
    @SenatriusPosted over 4 years ago

    Right, so I did just notice the updated version as I posted the comment on the old version. Here we go again.

    The icons are still going outside the card at width 1150px and then again at 895px. Using flexbox on each of the cards and then aligning all the elements within using align-items and justify-content instead of using direct values should make it more responsive. There's only so many media queries you can add for every screen resolution until you start losing your mind. Trust someone who did that before :)

    However, it is a clear and significant improvement over the last upload. Keep at it and you'll be building websites with the best of them sooner rather than later :)

    1
  • Johnny416•20
    @Johnny416Submitted over 4 years ago

    Hi! I've been learning HTML & CSS for around a month now, however I only did projects in a code-along way. I thought I would give "solo coding" a try. I would appreciate any feedback! Thanks in advance and have a nice day everyone!

    Four card feature section - The first website I made alone

    1
    Gediminas Makutenas•765
    @SenatriusPosted over 4 years ago

    Hello, and welcome to Frontend Mentor :) Made a good decision coming here, you'll learn a lot more a lot faster than you would have just by watching some videos.

    Now, onto the review.

    1. Looking at the report, it's a good idea to align all the images using CSS instead of aligning it directly in HTML. Whether you use position: absolute or display: flex or something else is up to you. You've got a few options.
    2. To make the website more responsive, you should add another media query at width of 880px, as currently the text wraps and the icon goes outside the container and only fixes itself at width of 425px. Reducing the margin between text and icon in the cards might work as well as switching to a single column layout a bit sooner.
    3. Just to make it a little nicer, add some margin to the bottom of the container like how you have it at the top.
    4. Code quality in general looks pretty good to me, easy enough to read and understand, can't see anything particularly wrong there.

    Keep up the good work and keep coding :)

    1
  • Gediminas Makutenas•765
    @SenatriusPosted over 4 years ago

    Hello there.

    When it comes to JavaScript I don't have much to say as I'm still learning it myself. From what I can tell it works fine though :)

    As for design: -Responsiveness works well on all screen sizes. Well done. -Professional card gradient is going top to bottom rather than left to right. Something to look into :) -Text in the cards should have font weight of 600, it seems a little bit thin right now.

    The code looks pretty good as well, I like the comments, makes it a lot easier to read. Might have to start using them myself more often.

    Keep up the good work :)

    1