2020 Tribute_Page_Feedback

Code Feedback
1

Hello Everyone. I had previously completed these challenges in Jan of 2019 – However, I was much more focused on the actual tests passing rather than making the site pleasant to the eye.

I’ve decided to redo all of my projects in Jan of 2020 and become more active on the FCC forums/twitter dev community.

I’d very much appreciate any feedback on my Tribute Page. Design, Code etc.

Thanks for your time

2

It looks amazing! Did you make the graphic? Not sure about the ordered list centered.I like the borders. Just finished mine. Its hilarious but crappy. I learned a lot.

1 Like
3

I think it looks great. I am not crazy about the numbers on the ordered list. You can get rid of those
with CSS. I also think changing the initial font color of the link at the bottom (before it changes on hover) would be good as it is hard to see!

I think this will get rid of the numbers on the ordered list:

.inventions ol {
  list-style-type: none;
}
1 Like
4

I’m not much of a designer — yet! I just did an image search and found one I thought looked nice.

I think I will remove the Ol style. How would you style the list if not in the center?

6

That’s the right CSS, I was going for the numbers on my initial design but after thinking on it longer I guess if my h3 is saying “top 10” there isn’t much of a reason to keep those numbers.

That stinking link within my button was being a pain to style - I like your idea on changing the default color before hover — I spent so long on the hover I neglected the default style.

Appreciate the feedback!

1 Like
7

It’s a decent looking page. I like how the switch from two to one column looks for the info boxes (see point 7 though).

  1. You have invalid HTML, e.g. nesting an <a> element inside a <button> is not valid. Run your HTML through a validator.

  2. Give the .info boxes some padding.

  3. Lower the margin on the body at small screen sizes.

  4. Remove the default padding on the <ol>.

  5. Invert the link color on hover (right now you would do it by targeting the link on button hover, i.e. from what you have .btn a:hover to .btn:hover a). But as I said it isn’t valid HTML so you should just remove the button.

  6. Line: 100 you forgot to close the media query.

  7. You do not need to use flexbox when switching to a one-column layout you can just change the grid-template-areas and remove the gap.

@media (max-width: 700px) {
  #tribute-info {
    grid-gap: 0px;
    grid-template-areas:
      "hero"
      "info-left"
      "info-right"
      "inventions";
  }
}

Good job keep it up.

2 Likes
8

Thank you so much!!! I really appreciate you taking the time to dissect this for me.

I was unaware of nesting the A tag inside of a button as bad practice. I’ll definitely fix that!

I’ll make those adjustments on styling as well as removing flex and fixing it with the appropriate grid layout. I was definitely struggling with achieving that one column layout with grid which is why I resorted to flexing it. Thanks for adding the code snippet.

9

That has all been corrected. I didn’t realize that the OL had default padding, I was really causing a headache trying to figure out why my link seemed off center. – That fixed it.

Thanks again for taking the time to respond.

10

Center is good. my eyes felt like they wanted to go there, but the numbering on the left was distracting, and I had difficulty lining the number with the text. So numbering centered along with text? color-coding? freeform/collage. I’m working on the survey right now and its kicking my butt, haha.

1 Like