Skip to content
OverviewSolutions
4
Comments
12
Fran Extremera
@franexmo81

All comments

  • Fran Extremera•210
    @franexmo81

    Great job with this challenge. Nice and clean.

    I've only noticed a couple of things that could be improved:

    • User info (name and Verified Graduate) are aligned on left on the design, but appear centred on your solution.

    • You could have grouped some properties for the profile images, like this:

    .profile-image{
    height: 28px;
    width: 28px;
    border-width: 1px;
    border-style: solid;
    border-radius: 50%;
    margin-right: 13px;
}

.dan {
    border-color: var(--White);
}

.jon {
    border-color: var(--Moderate-violet);
}

    Also, remember to replace Your name here on the footer 😉

    Happy coding!

  • Fran Extremera•210
    @franexmo81

    Really nice job.

    The solution looks as the intended design and the responsiveness works perfectly.

    If you want to, you could simplify your CSS a bit by grouping similar elements with the same class name. For example:

    Here is some code from your solution:

    <div class="one" id="one">
<div class="two" id="two">

.one {
  background-color: var(--moderate-violet);
  background-image: url(./images/bg-pattern-quotation.svg);
  background-repeat: no-repeat;
  background-position: 90% top;
  border-radius: 10px;
  overflow: hidden;
  padding: 1.5rem;
}

.two {
  background-color: var(--very-dark-grayish-blue);
  border-radius: 10px;
  overflow: hidden;
  padding: 1.5rem;

    What you could do is assign an additional common class to both divs that contain the common properties, like this:

    <div class="card one" id="one">
<div class="card two" id="two">

.card{
  border-radius: 10px;
  overflow: hidden;
  padding: 1.5rem;

.one {
  background-color: var(--moderate-violet);
  background-image: url(./images/bg-pattern-quotation.svg);
  background-repeat: no-repeat;
  background-position: 90% top;
}

.two {
  background-color: var(--very-dark-grayish-blue);

    This could be applied to the rest of the elements (userone/usertwo, infoone__p__s/infotwo__p__s ...)

    This brings some advantages. Let's say that in the future you want to modify the padding for all the cards. You would only need to modify it in one place.

    I hope you find this useful and have a successful learning.

  • Fran Extremera•210
    @franexmo81

    Nice submission, really accurate.

    I've just noticed some misbehaviour in the responsiveness:

    • The cards are not vertically centred on the screen.
    • The text overflow the cards on screens between 795 and 1250 px width
    • The violet card doesn't take the full width on screens narrower than 795 px

    I think that most of the above can be fixed by just doing:

    • Remove padding: 70px for main-wrapper
    • Replace height: 100vh with min-height: 100vh for main-wrapper
    • Remove grid-template-rows: 300px 300px for testimonial
    • Remove width: 300px for the violet card under 795 px width
    • Remove margin-top: 500px and margin-bottom: 500px for main-wrapper under 795 px width

    I hope the above helps you in your way to become a successful developer. Good luck!

  • Fran Extremera•210
    @franexmo81

    Nice job. The grid layout works as intended and is responsive.

    However, I'm afraid that the font type is not properly linked. In Index.html you have: <link href="https://fonts.google.com/specimen/Barlow+Semi+Condensed" rel="stylesheet">

    But, actually it should be: <link href="https://fonts.googleapis.com/css2?family=Barlow+Semi+Condensed:wght@500;600&family=Roboto:wght@400;700&display=swap" rel="stylesheet">

    Please, have a look to how to use Google Fonts to understand why the above is needed.

    Also, at the CSS, it's recommended to place the font type between quotes, and provide a more generic second option. I mean, instead of what you have: font-family: Barlow Semi Condensed;

    To have this: font-family: "Barlow Semi Condensed", sans-serif;

    I hope these changes helps. Happy coding!

  • Fran Extremera•210
    @franexmo81

    Good job overall.

    Just some things that don't look completely OK:

    • The background colour doesn't match with the design
    • The cards are not vertically centred on the screen
    • There is no gap between cards when width is less than 600 px.

    Also, I would personally improve the footer although it's not part of the challenge.

    All above are small details that I'm sure you can easily fix and would lead you to a perfect page. Well done!

  • Fran Extremera•210
    @franexmo81

    Congratulations for such a great job. It looks amazing and works perfectly.

    I've only notice some few details that maybe could be improved:

    • The shadow when hovering on the 3 (or 5) options to choose would look better with some transition
    • You could take advantage on the link for your name ("Sebastian") at the footer by making it point to your personal webpage, or Github profile...

    Other than that, excellent job. Keep it up!

    Marked as helpful
  • Fran Extremera•210
    @franexmo81

    Hi RoRajak and congratulations on submitting this solution.

    Don't be so hard with yourself ("I made lots of mistake") 😊.

    The layout looks quite well and accurate on desktop view. However, it looks like you struggled a bit with the mobile one. I'm sure that if you keep trying, you'll be able to improve. It's all part of the process.

    Here are some other easy fixes that would help:

    I've noticed that the intended font is not working.

    Instead of linking from the TTF file: @import url(/results-summary-component-main/assets/fonts/HankenGrotesk-VariableFont_wght.ttf)

    Try better with importing from Google Fonts: @import url('https://fonts.googleapis.com/css2?family=Hanken+Grotesk:wght@500;700;800&display=swap')

    Also, it looks like the images are not loading properly.

    Try to replace the way you've linked: src="/assets/images/icon-reaction.svg"

    With this way: src="assets/images/icon-reaction.svg"

    I hope all this helps. Good luck and keep going!

    Marked as helpful
  • Fran Extremera•210
    @franexmo81

    Your solution looks really similar to the design. The responsiveness seems to work nicely. Well done!

    I would have named the CSS variables in such a way that their name is not referred to the color but for their use. Something like "primary-color" or "accent-color", etc.

    The idea behind it is that if the colors need to change in the future, you can simply modify their value and they will keep their meaning without needing to modify their names all along the file.

    Bonus tip for a better appearance: Some transition for the button hover status.

    Marked as helpful
  • Fran Extremera•210
    @franexmo81

    Hi!

    It looks good, but here are a couple of easy wins that could easily improve it:

    • Instead of DIV, make it a BUTTON and give it a HOVER effect.
    • Increase the @media query max-width to about 700px to improve responsiveness

    Bonus one: Some shadow on the card as per the example.

    💪

    Marked as helpful
  • Fran Extremera•210
    @franexmo81

    Here is an example of SVG changing colours when hovering:

    Inline SVG Hovers

    Basically, you need to include the SVG definition inside the html document, instead of linking them from it. Then you select it and modify fill with :hover pseudo-class

    I hope it helps

  • Fran Extremera•210
    @franexmo81

    Hi Alessandra. Nice looking job.

    However, I've noticed that the card doesn't stay in the centre for screens wider than 1440 px.

    I believe the reason is body has max-width so it doesn't fit the whole width of the screen and stay on left side.

    In any case. Good work, keep it up!

    Marked as helpful
  • Fran Extremera•210
    @franexmo81

    Hi Ben, your solution looks really good.

    I just noticed that the buttons lose their shadow colour when hover over them.

    Nice job!