Hi, can someone critique my Tribute Page?

Code Feedback
1

Hello all, I just would like your opinion and do a lookover of my FCC Tribute Page.

FCC - Tribute Page

2

I love that you have a separate frame for the timeline that aligns with the picture. Very sophisticated. Your page is freakin awesome.

3

Also, that footer is a really nice touch.

4

Thanks, lepros. That footer I’m putting on every one of my FCC projects.

5

I think It was not necessary use div with footer id attribute just footer tag was enough.
For named of class or id use meaningful nouns. for example class with name “fcc” is not good

6

I think it’s good, well done.
There is one little thing I would change for smaller screen sizes ie mobiles, your tribute info section becomes too thin below 500px width especially in the 300-400 range. I would remove the left right margin entirely at that point to give it more width.
There would still be some margin from the main element, personally I would probably remove the scroll bar below 500px and give the tribute info the full width

7

Ah, yes. I’ll correct that one.

8

Hmm, you’re right. Once the tribute info slides down to the bottom, the scroll bar should go since it seems odd to have it there.

9

If think the scrollbar looks ok on normal screens but takes up valuable space on a narrow mobile screen. I think you actually can to some extent control the thickness of a scrollbar with css though I haven’t looked into it much yet.
I would just remove it below 500px I think. Your tribute page is actually very similar to mine… great minds think alike :laughing:

10

Problem with the media query in Chrome and Edge though. If I go below 1559px the tribute info goes to the bottom, the scroll is still there (even though I’ve switched to display:block and overflow:initial). If I keep going to ~1542 then the scroll bar goes away.
The problem is not present in Firefox. Hmm…

11

One issue with making the list scrollable is that except for Firefox the content in the list is not keyboard accessible by default because you don’t have any focusable elements in it, so people who use the keyboard will not be able to scroll the content. Personally, I would not make the list scrollable but rather let it take up all the height it needs. There is already a main vertical scroll bar for the entire page so having a secondary vertical scroll bar for just the list seems unnecessary. But if you do want to continue to make the list scrollable then you’ll need to make it keyboard accessible by adding tabindex="0" to the <section> tag doing the scrolling. Furthermore, any element that can receive focus must also have an accessible name and role, so you would need to add role="region" and aria-label="Timeline" to the <section> as well. But really, I would just allow the list to be as tall as needed without forcing scrolling.

12

Thanks for the feedback. Great food for thought.

13

For me messing around with your code to see if I could get it how I would want it looked something like

@media (max-width:500px) {
#tribute-info {
overflow:visible;
margin:3rem 2% 0 1%;
height:auto;
padding:1rem;
}
}

Would have put this in a quote or code block but it refused to do what I wanted :slightly_frowning_face:

14

Nice job! I can see that you tried to make this responsive. It’s working pretty well!

Okay, so there are some tips I’ll share with you to help you improve on it a little bit.

  1. Remove the white border around the main body.
    Screen Shot 2021-09-13 at 11.33.12 AM
    Screen Shot 2021-09-13 at 11.33.12 AM1920Ă—801 87.5 KB

There is a little white border surrounding your webpage and that makes the website look less professional, sort of an embed. By adding this code to your css, that should be fixed up really quick.

html, body{
  margin: 0;
  padding: 0;
}
  1. Give the timeline a different color or a border
    Screen Shot 2021-09-13 at 11.35.38 AM
    Screen Shot 2021-09-13 at 11.35.38 AM1088Ă—706 67.9 KB

Having a scrollable list in the body is a good idea although usually that wouldn’t be a good way to do it.
I would suggest making it just a part of the document by letting it retain its full height or making it stand out a bit in the webpage.
Having a little “pocket” like that looks a bit awkward (the scrollbar in the middle of the screen and all) and also, some users might not even choose to scroll down and view it due to its unusual appearance there.

  1. increase the timeline width in mobile view
    Screen Shot 2021-09-13 at 11.38.21 AM
    Screen Shot 2021-09-13 at 11.38.21 AM400Ă—684 14.3 KB

The timeline looks really squashed up when the screen width is smaller. Try adding a max-width css property or use a media query to make it take up full width when the window’s width is smaller.

Overall, you did a great job. Keep up the great work!

1 Like
15

Ok fellas, I’m going to look over this again.

16

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.