Skip to content
OverviewSolutions
12
Comments
6
Miguel Chavez
@toonchavez8

All comments

  • Miguel Chavez•210
    @toonchavez8Posted almost 2 years ago

    I'd like to congratulate you on completing the challenge; well done! Getting started can be daunting at first! Since you tagged this as #react #redux and #react route, I was curious to see how using react on this challenge would work. If this is the case, then perhaps you could update your tags if the repo is correct, otherwise, I'm not sure whether there might be confusion.

    Secondly, in terms of your code, I took the liberty of reviewing it and in terms of the accessibility report, your HTML is missing 1 landmark element.

    1: the main tag you should wrap your code after 19 and before line 30 in your index HTML and that should get you passing the accessibility report on here.

    Ive been a fan of using import in css rather then link tags in html as ive been under the understanding that link tags while usefull will load before our markup this means that the more links we have and use the longer the page will take in providing content, i would suggest removing the link tags for the font and adding it yo the top of your css something like this

    @import url(https://fonts.googleapis.com/css2?family=Outfit:wght@400;700&display=swap');

    other than that i believe the only other things i might suggest to modify are sizing but that at the end of the day thats really not game-breaking. i did check across multiple devices and your media quiery holds up so good job there!

    Marked as helpful
    0
  • Miguel Chavez•210
    @toonchavez8Posted over 2 years ago

    Hey good morning, first of all, I want to congratulate you for submitting a solution I know firsthand how much of a task it can be! so great job for accomplishing it.

    I took the liberty of pulling your code and I've reviewed it. and here are my suggestions in order to make some improvements.

    • in terms of expected markup language for HTML your missing <title>Your title </title> in the <head></head> section at the top of your index.html

    • there's a small error in your img that while it would make it work, some browsers might not detect the source linking, The error is a Backslash () <img class=img src=images\image-equilibrium.jpg`` and the best practice would be <img class=img src=images/image-equilibrium.jpg

    • In terms of accesability - you have a <div class="content"> rest of your code</div> wrapping your content you could easliy fix the accessability issues by swaping it for <main class="content"> rest of your code </main>

    • also all pages should have an <H1></H1> i would swap out your <h2>Equilibrium #3429 </h2> in line 31 for h1 and that would also fix the other accessibility issues

    if you want to read more on accessibility and semantic HTML I will provide a link Semantic HTML

    Now onto the fun part styling

    • You have display flex on your .content and that's great for adjusting most things but in this case, you wanted to center the card on the page and forgive me if I'm wrong but i haven't managed to make it work in the past with flex, however with position:absolute; I've managed to center it perfectly.
    position: absolute; /* this will activate top right left and bottom functions */
top: 50%; /* what this is doing is linking the content to 50% of the top and sending it to the bottom it's inverse i know */
left: 50%; /* this is the same as above but to send it to the right */
transform: translate(-50%, -50%); /* this is is negating top and left making stick to the center of the page*/
}

    • in terms of landing the design you got very close, however i was to knit pick i would reduce the dropshadow by adding rgba like this to make it more subtle box-shadow: 0px 10px 10px 5px rgba(0,0,0,0.1);

    • similar to the way that we centered your card, we can place your attribution to the bottom of the page and have it stick there. footer{ position: absolute; /* this will activate top right left and bottom functions */ bottom: 0; /* this is sending it to the bottom */ left: 25%; /* lightly to right */ width: 50%; /* this is the width of the footer */ height: 50px; /* this is the height of the footer */ text-align: center; /* this is aligning the text to the center */ font-size: 15px; color: hsl(215, 51%, 70%); }

    • also general recommendation on your css i see you are using display:flex; however im not seeing any other sub-tags like align, flex-direction to name a few, sometimes it's not necessary of course, but in this case, there's a couple of instances where you could incorporate them to make sure if the user adjusts their screen that everything moves like you want it to. I would highly recommend you play Flexbox Froggy it definitely helped me grasp flex

    There are a lot of other ways to do these cards but this is what worked for me and it might work for you. Give it a shot and good luck!

    btw some of the code blocks on here look immense without the vscode styling, i highly recommend copying and pasting them onto a code editor and giving it formating, if you are using VSCode with Shift+Alt+F Format document or Ctrl+K Ctrl+F Format selection and it will make it more readable as I included comments in the code block that will describe what each css style will do to the best of my ability

    Marked as helpful
    0
  • Miguel Chavez•210
    @toonchavez8Posted over 2 years ago

    Hey there I'm always glad to see people submit solutions as I know understand how difficult it can be to get them done sometimes so congratulations for that!

    Firstly to answer your question I've always found it easier to to place it in your :root{}Incase you have to modify yourbody{} I can suggest giving this a read to get a better understanding on some ways to reset your css

    1
  • Miguel Chavez•210
    @toonchavez8Posted over 2 years ago

    Great job on submitting a solution! That's always a big step!

    I've reviewed your code and I have some suggestions that might help.

    Firstly I highly recommend creating a .css file and linking it using the <link> tag like this <link rel="stylesheet" href="style.css"> and place all of your CSS there to make your HTML more readable.

    In terms of HTML regarding accessibility, there are some good semantic articles I would recommend you take a look at like this one... Web.dev/landmarks

    the jist of the article is that HTML provides landmark tags that will help users identify what section of content they are reading, just as the <main></main> tag, I would wrap all of the components within that.

    Another point of the article is declaring titles or headings as their respective heading case, I would recommend changing some of your <divs> for H1 and H2 and then styling them according to the goal.

    in terms of css i see that you declared your #content with a display:relative and im not if you meant to go with a position:relative or a Display:Flex if its that latter you would need to declare it as display:flex; flex-direction:column; in for it to not change your current flow

    Marked as helpful
    0
  • Miguel Chavez•210
    @toonchavez8Posted over 2 years ago

    Hey buddy seems to be some confusion as to what I assume is just a mistake at the time of uploading your solution. The link provided to this is in fact to the demo page of the intended solution however the git hub link takes you to another page. not sure if that was done intently

    I recommend using Flexbox to allow your card to be responsive

    0
  • Miguel Chavez•210
    @toonchavez8Posted over 2 years ago

    I haven't done it so I can't speak to the code or the challenge you faced, however in the spirit of constructive criticism your solution seems smaller in comparison.

    After reviewing your code it is well structured from my POV however I'm not sure if there's a redundant code styling labeled my-2.

    All in all good job!

    1