Skip to content
OverviewSolutions
12
Comments
13
Kristina Radosavljevic
@KristinaRadosavljevic

All comments

  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi, Sara! Congrats on the solution, it looks pretty nice :)

    One thing I noticed is that your .card__main might not be centered in the best way. Instead of using margins to center it, I would suggest you try to use flexbox or grid with <body> as the container and then you can use align-items and justify-content properties to center the main content both vertically and horizontally.

    And also, it might not be the best idea to use fixed width and height because then the content doesn't scale well with the user's screen. My suggestion is to use max-width instead of just width and to lose the height altogether because then the height will be adjusted to the content.

    Hope this helps and once again good job :)

    0
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi there! Congrats on completing the challenge, it looks really good :)

    There's just one thing I noticed that you might want to fix - I would suggest using min-height instead of height on the <body>. Otherwise you can't really scroll on 'short' screens (where the width is big but the height not so much) and the content is cut off.

    Hope this helps and once again good job :)

    Marked as helpful
    1
  • Ssembatya Dennis Duncan•260
    @ssembatya-dennisSubmitted about 2 years ago

    Hi everyone 👋🏽, Am very glad to have completed this challenge and reach this far with the platform, however as I was doing this solution I used the transform: translate() CSS property to make that tilt effect on cards. I really appreciate if I get to know better ways of going about this challenge and how best I can improve my solution.

    Responsive Social proof section with CSS Flexbox

    1
    Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi and congrats on completing the challenge, it looks pretty good :)

    I did notice a couple of things that you might want to take a closer look at:

    • I would advise against limiting the height of the .testmonial__card, because then if the text gets too long, it's cut off, and if it gets shorter, it leaves a lot of blank space at the bottom of the card.
    • I would consider putting the media query a lot sooner than 425px (or using another breakpoint in between). The layout starts to break as soon as around 1000px.
    • As for your question about using transform: translate(), I personally don't see any problem with using that - it looks fine and it scales well. In my solution, I used margins to push the elements, but I honestly don't see any significant advantage of using one over the other :)

    I hope this helps! :)

    Marked as helpful
    1
  • Attrams•130
    @AttramsSubmitted about 2 years ago

    Hey guys, this is my solution for this challenge. I had trouble positioning the icons in the cards and after googling I found a solution but I don't understand why the icons move outside the card when I comment out position: relative in .card. Full code can be found below. Thank You.

     .card {
  width: 100%;
  height: var(--mobile-card-height);
  background-color: white;
  margin-bottom: var(--card-margin-bottom);
  border-radius: 0.5rem;
  padding: 1.4rem;
  position: relative;
  box-shadow: 0px 0.875rem 1.25rem -0.313rem rgba(218, 228, 240, 1);
}

 .card img {
  position: absolute;
  right: 0;
  bottom: 0;
  padding: inherit;
}

    Responsive site built with FlexBox

    1
    Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi Attrams! Good job on your solution :)

    As for your question, when you specify position: absolute;, the element is positioned in relation to the nearest parent which is also positioned (i.e. which has the position property set to anything other than static, which is the default). However, if you don't have such an element (which happens when you comment out position: relative; from .card), then the first parent in the hierarchy (which is <html> I believe) is used as a reference for positioning the icons. You can read up on this all over the internet if you just google 'css position absolute', but I also found this article, which goes into a lot of detail from the beginning so it might be good as a starting point if you want to learn more :)

    Also, while we're at it, I noticed that the titles and the introductory paragraph are a bit too narrow from screen widths of about 1310px to about 900px, so you might wanna take another look at that.

    I hope this helps :)

    Marked as helpful
    0
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi there, and congrats on completing the challenge :)

    I looked through your code and I have a couple of suggestions that you might want to consider:

    • You could try to get this solution a bit more responsive. A few ideas to start with - don't set a fixed width for the <body>. This will make your layout overflow to the sides (which is never a good idea) even if your user's screen is only a couple pixels less than 1440px. Another best practice you should adopt very early on is to get comfortable using responsive units like rem instead of fixed ones like px. And finally, I think you can set the media query breakpoint earlier than 375px - it's always best to look at your layout and see where it starts to break.
    • In your HTML, I'm not sure I would use <h2> for that description. Headings should be used as titles for a section, etc. This looks like a plain old <p> to me :) Also, the Register button is probably supposed to take you to another page where you can register, so in this case I believe the <a> element is more appropriate (even though it may look like a button).
    • For the sake of better user experience, I'd suggest adding the cursor: pointer; property to all the clickable elements.

    Hope this helps and once again good job :)

    Marked as helpful
    0
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi and congrats on completing the challenge :)

    I think I have a couple of suggestions that you might want to consider:

    • 1440px is way too much for a media query breakpoint for this project. I understand you did a mobile-first approach which is perfectly fine, but I would put the breakpoint much sooner (maybe around 600px or something - whenever the layout breaks and the columns start to look stretched out).
    • You have a great start with the button, but it could use a bit more work. Most notably, you should have some sort of hover effect on it, just to indicate to the user that it's clickable (maybe a background color change, or simply using cursor: pointer; would do the trick). I also noticed you used margins to position the button and it starts to look displaced after a certain point (around 400px and smaller) - might be a better idea to either stretch it so that it covers the entire width of the parent element or to use margin-left: auto; and margin-right: auto; to position it at the center or (and this is probably easiest) not to use left and right margin on it at all. And speaking of the button, since clicking it would presumably take you to another page (where you can sign up), I guess the right HTML element to use would be <a> and not <button>.
    • I noticed in your code that you used a mix of px and rem units, which I guess is not the best practice. You should stick to one unit for consistency, and my suggestion is to go with rem since it's the responsive one :)
    • One last thing, and this one is pretty tiny - I think the paragraph in the "Why Us" section is actually supposed to be a list, just without the bullet points. It is pretty packed in the design so I see how that could be missed.

    Sorry for the longish feedback, but I do hope some of this helps :) Cheers!

    Marked as helpful
    1
  • Attrams•130
    @AttramsSubmitted about 2 years ago

    Hey guys, this is my solution to this challenge. I noticed that the container expands a little when I hover over the button, is there a way to prevent that? Thank You.

    Responsive site built with FlexBox

    2
    Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi @Attrams, congrats on your solution, it looks really good :)

    As for your question about the button getting bigger on hover, that happens because you're adding a border on hover, so that expands the whole element by whatever is the width of the border. Since the background color on the inactive state is the same as the border color on the hover state, I guess the simplest solution you can try is to add the border for all states (not just on hover).

    Another tiny suggestion I have is to maybe put a smaller margin on the .container element in mobile view - as it is now, I think the container starts to shrink a bit too soon and gets very narrow on mobile. This might be just a personal preference :)

    I hope this helps and once again, really good job on this challenge :)

    Marked as helpful
    0
  • SIRI.T•30
    @MissSiriluckSubmitted about 2 years ago

    🐣 My 2nd Newbie(free) project: Please help to advise. Thank you for any feedback. 🌻 ✌️

    QR-code-component

    3
    Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hey, great job on this project, your solution looks really good :)

    There are just two slight adjustments that you might want to consider making:

    • You don't have to use so many <br> elements in your HTML - the text will break naturally once the proper layout is set up.
    • I guess the centering technique you used with position: absolute; and translate() works in most cases, but it fails a bit when you shrink the screen vertically (which would happen if you view your project on mobile in landscape mode, for example). So, I and many other people here opted for making the body a flex container and centering the <main> element by using align-items and justify-content properties. You would also need to set min-height: 100vh; on the body for this to totally work.

    I hope this helps and once again, good work! :)

    Marked as helpful
    1
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi there, congrats on completing the challenge :)

    As for coloring the image, there's a CSS property called background, you can assign it to a color, image, gradient, etc. And also, this property can have multiple values, where the values will stack on top of each other. You can use this functionality to very easily add a tint to the image by overlaying it with a semi-transparent linear-gradient (I used this article to help me figure it out, and you can also check out my solution to see how I implemented it).

    There are a couple of other little things I noticed while checking out your solution:

    • I don't think you should use this many <br> elements in your HTML. I believe the text is supposed to break naturally when using the correct layout.
    • Instead of using .stats ul li:nth-child(1) as a selector, I would personally give these elements a class name, I figure it would make the CSS code a bit cleaner (although, I guess this can be used to practice pseudo-classes, so if that was the intention, you can ignore this point) :)
    • You can work on making your project a bit more responsive by using things like rem units (or some other responsive units), max-width, media queries, etc.

    I hope this helps! :)

    Marked as helpful
    0
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi there,

    Just letting you know that your link to GitHub isn't working.

    As for the solution itself, what I noticed is that you're missing the hover effect on the Change link (it's supposed to turn purple and lose the underline). Also, I would probably add the cursor: pointer; property to all the clickable elements, I guess it's just good practice from the user experience perspective. Other than that, I think it looks pretty good :)

    Hope this helps and please fix the GitHub link so that people can look at your code and give you more in-depth feedback :)

    Marked as helpful
    0
  • Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi Max! Congrats on completing the challenge, your card looks really good and close to the target design :)

    I did notice one thing that you might want to fix - the overlay on hover on the main image doesn't overlap with the image but is way down, at least on my screen. I assume this is because you used position: fixed; on it, which makes it super hard to adjust to different screen heights.

    The way I did this (maybe there are some other/better ways), is to put the main image and the overlay within the same container, and then you can use position: absolute; on the overlay to position it in relation to the parent element (as opposed to the viewport). This way you can ensure that the overlay is right on top of the image regardless of the screen size. Feel free to take a look at my solution if you need more context.

    Hope this helps, and once again good work! :)

    Marked as helpful
    0
  • Raymond Fajiculay•230
    @keyztrokeeSubmitted about 2 years ago

    Hi! I would appreciate it if you could review my code and give feedback. Your feedback is very important to me, thank you very much!

    NFT preview card component

    2
    Kristina Radosavljevic•380
    @KristinaRadosavljevicPosted about 2 years ago

    Hi, Raymond! Very cool solution, it really looks great :)

    I noticed two things that you might want to consider:

    • I would add just a little bit of top and bottom padding on the body because the card sticks to the top/bottom edge of the screen if the viewport height is smaller than the height of the card.
    • A little disclaimer for this next one - I don't know all that much about Sass, I'm just starting to get into it myself - BUT I think there's a different way to define variables in Sass and it's much cleaner than the regular CSS way. You can define them as, for example, $cyan: hsl(178, 100%, 50%); and you don't have to do it inside the :root selector. And then when you use them in the code, you don't have to use the var() function, but just the name preceded by the $ symbol, as in background-color: $cyan;. Apparently, however, there's a difference in how CSS and Sass variables are compiled, so maybe read more on that (for example here) before you take my advice :D

    Other than that, really great job! :)

    Marked as helpful
    0