Skip to content
OverviewSolutions
7
Comments
7
Clinto Bean
@clinto-bean

All comments

  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Good work on the positioning and styling. A couple of pieces of feedback.

    • On the main card itself, the entire thing has cursor: pointer, so even if you are not hovering over an interactive element (i.e. just the text within certain sections), it has a cursor. This indicates to the user that it is interactive when it is not. You are better off declaring cursor: pointer; on the hyperlinks themselves (using the a{} styling in CSS) and doing the same for the .image-container.

    -- Within the .image-container, you have the following code:

    .card .image-container:active::after{

    display: block;

    }

    you don't necessarily need to add .card before this as you are already referencing the .image-container itself. You don't have to reference parent elements unless you are trying to override other specificity.

    --- You also will want to add a hover state to the image container. You can do both with one CSS code block by using a comma between selectors. See below.

    .image-container:active::after,

    .image-container:hover::after {

    display: block;

    }

    Let me know if this comment was helpful. Good job on completing the challenge and keep up the good work!

    0
  • Thomas-Nigon•30
    @Thomas-NigonSubmitted over 1 year ago

    Hi guys this is my first project. I'm learning on my own before starting a 5 month course in September.

    It's far from Pixel perfect I know that. The scaling is off and colors/font size/font weight are not matching. I know it's important but I think it's better to focus on the big picture instead of having a pixel perfect project that lack essential features.

    For this project I had a hard time to: _Adjust the text position especially inside the circle _The footer (that doesn't show at all with vercel - why? I can see it on all my previews is vscode and chrome dev mode) did not went to the bottom of my body. I tried to have the footer inside or outside of the body or main flex container. _I have a hard time in general with the scaling (units, font size and when to use for say height min/max-height) to fit responsive requirement. _Lastly I think I struggle with my html arrangement and when or where to use a tag or a class/ID. (Like I said text position was a problem sometimes, so i'm not sure I did things properly)

    My next step is to add the JS and fetch the data from the json file; Same a the general looks of the project I felt that being close enough manually was better that struggling for 2 days on fetching the data.

    Thank you for reading me, Thomas.

    Result summary Thomas NIGON

    1
    Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Hey Thomas, good work on this.

    • For the circle, what I think would help is to structure the circle itself as a container, with the text being its contents, rather than trying to add borders to the text elements. Something like this may be helpful:

    <div id="circle">

    <div class="score"><span>76</span> of 100</div>

    </div>

    Then,

    #circle {display-flex; flex-wrap: wrap; }

    This sets the circle element as a flex container and wraps the items vertically should they exceed the width of the parent.

    .score span {font-size: 40px, color: white }

    This selects only the text within the span element (76) and applies a larger font size and color to it.

    By default, the footer will not go to the bottom of the page, since your body element does not take up 100% of the viewport's height. You can change this by adding height: 100vh if you want to, but that's not necessary.

    For sizing and positioning, I definitely recommend reading through the MDN documentation on it or watching tutorials from someone. Particularly, I watch Kevin Powell, but there are countless great creators who are invaluable resources for learning.

    Marked as helpful
    1
  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Hey Matthew, great job on this. It's very responsive on both my laptop and mobile. Your layout is almost 100% perfect - it seems there's some padding on yours that makes it slightly smaller than the original design but other than that it is 100%.

    The only improvement to the code I can see in your App.jsx is that there are some cases where the classname is written in camel case with the first word capitalized, while there are others where it is not. For example:

    className="CardRightContainer" and className="containerCardRight", typically the tradition in JS is that you would not capitalize the first word, but if you wish to do so it may look better to do it on all of them.

    Another thing you can look to is to create a component for each of the score bars which would map over each category to create the bar, this would make it easier to add or remove rows if a new category were added or removed.

    Great work!

    0
  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Hey Jordan, great work. The only thing I can recommend is to avoid using the min-height: 100vh on the body, because when viewing this on my laptop (1920x1080 @ 13"), it is vertically overflowing causing scrolling that isn't necessary.

    You can either attempt to size the main body by adding padding to the container and its children, or you can use a unit like dvw rather than vw, as dvw is a dynamic viewport unit that takes into account things like the UI elements of a user's browser.

    0
  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Hey Anshul,

    Your code for the overflow: hidden on screens below 376px seems to be correct. When I load the page, I don't have any overflow. One thing I can recommend, in your browser, is to use the element picker (Shift + Ctrl + O on Windows Firefox) and hover over the elements, where you will see the full width it takes, and if something shows larger than 375px, then you may have found your culprit.

    Something you will find helpful in the future is to not use static width and height (like width: 18rem), just max-width, and rather size the inner elements using things like padding and margin, this makes it so that your website is more easily made responsive.

    Marked as helpful
    0
  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Great work on making this responsive! One thing you may want to consider with the media queries, while shrinking down to a large phone/small tablet size I noticed that the summary rows became larger vertically because they were forced to wrap around the scores.

    One thing which you may benefit from is to create a wrapping div around the scores to have the parent element see it as just one item rather than three. Something like this would probably be an easy implementation.

    <div class='category visual-category">

    <svg ...></svg>

    <div class="score-wrapper">

    <p class="category-label"> Visual</p>

    <p class="score"><span>72</span> / 100</p>

    </div>

    </div>

    Note the removal of the h4 element as well. With semantic HTML, we want to avoid reusing specific heading levels for the purpose of screen readers, and instead use CSS styling to take care of it. You can easily apply your styling to all of the score categories and scores themselves with the following CSS.

    .category-label {

    font-size: 1.25rem;

    font-weight: 700;

    }

    .score span { font-weight: 700; /* Alternatively you can choose to use font-weight: bold. */ }

    Hopefully you find my feedback helpful. This was the first challenge I took on and I definitely had my head wrapped around it for a while. You did a great job and I hope to see you continue working on these challenges.

    0
  • Clinto Bean•130
    @clinto-beanPosted over 1 year ago

    Hey, this looks great. It's very responsive and all the animations are very smooth. I like the effect of the added shadows on the main sections as well.

    One thing I would like to offer as feedback is that the sub-title for the hero section seems to "drop in" from the hero image, which is a bit conflicting on the eyes.

    Something that may have been better on a load animation is just to have it transform from 0 to 100% opacity, or possibly transition in from the side while maintaining a lower z-index from its neighbors so that it doesn't overlap anything. Overall, great work.

    0