Tribute page, review please

Code Feedback
1

hi guys,

I have a tribute page to review, every feedback welcome! May force be with You!

3 Likes
2

I think this is really great! I have a lot to learn from your code (just checked out the <figure> tag for the first time!).

Since the cafe terrace and almond blossoms photos lack .bottom-line, it feels like the captions are squeezed a bit tightly to the pictures. I think you did that because you didn’t want the bottom border that separates the other pictures. Also, galery should be “gallery” in the link at the bottom of the page.

Very small things i noticed.

Was impressed by the choice of background color, as well. In a tribute page to a painter, it feels like colors will matter a lot more, and you chose a good one!

3

I really like this, I remember learning about Van Gogh early on in school and always enjoyed his style.

I would have liked to have had some text with some facts about him. Also the last two images lack the underline which makes it look incomplete to me. The text under the portrait of him is to similar in colour on my screen.

4

Regarding the responsiveness, instead of reducing the size of the paintings, reduce the space between them.

Maybe recode the project with css grid and/or flexbox.

5

Thank you for feedback,

I will change few things as you suggest and I need to do something with responsiveness… :slight_smile:

6

@HaiCia Very nice! All the tests pass successfully. :fireworks: Your visual design skills really show in this project. I also like your use of HTML5 semantic elements! Here are some issues I found that you may want to implement in order to make this even better:

  • Section vs. Article: I’m not sure if using the article element is best here. My gut says the section element is better and maybe, wrapping each figure inside an article element if needed. I’m not sure though. What was your reasoning for using the article element instead of the section element?

  • Mobile-First Design: When I look at the site at 320px width (iPhone 5 and other smaller devices and viewport widths), it looks like this:

    screenshot-320px-width
    screenshot-320px-width322Ă—700 80.3 KB

    I think this can be fixed by taking a mobile-first approach to design where your base CSS is meant for small viewport widths and you add media queries for minimum widths to account for changes to larger viewport widths. I’m only speaking about the media queries at this point as there is a lot more to mobile-first design than the CSS. This article explaining mobile-first and responsive web design was very enlightening for me.

  • Blockquote instead of div: In your code, you have <p class="quote">. I think it’s more descriptive for screen readers and search engines to use the <blockquote> element instead: <blockquote>: The Block Quotation element - HTML: HyperText Markup Language | MDN.

  • Main Element: If you use the main element instead of <div id="main">, you can gain an interesting accessibility benefit, as mentioned in the Jump Straight to the Content Using the main Element challenge:

    The main tag also has an embedded landmark feature that assistive technology can use to quickly navigate to the main content. If you’ve ever seen a “Jump to Main Content” link at the top of a page, using a main tag automatically gives assistive devices that functionality.

Those are the big things I see. Great work overall! :sunny:

7

thanks @camper for yours review!

really good questions, when I started to answer them in two or three times I was wondering why I did it :smiley:

  • I used article because in my opinion article is a self contained tag which is ok in here(I may be wrong), I have no sections in my tribute like “about Vincent”, “his paintings” etc, I would use sections in this case. But yes you are right it is not article at all I didn’t realize it.

  • I have already changed few things about responsiveness and I focused on images totally forgot about fonts.

  • instead of class=“quote” i could just style <q> i can’t remember now why I use class…

  • last thing main. I made it this way because fcc wanted element with Id=main, in my tribute page whole page is main in my opinion so I not need main tag at all.

Yours review make me think about few things :slight_smile: thanks!

1 Like
8

@HaiCia How about something like this:

<main id="main">

That way, you pass the tests and get the semantics and accessibility bonuses. :star: