My Tribute Page, all feedback welcome

Code Feedback
1

This is my attempt at the Tribute Page and was hoping to get some feedback. I feel my code (particularly the CSS) might have a little too much and could be consolidated a little.

Many thanks for looking

2

Hi Paul,
very very nice job, I really like this design.

• You may want to change your wikipedia link color (this blue is not very visible).
• In my opinion your id=“sub-text” is a little bit to dark as well, but that’s just personal.
• last thing I would change is make a little bit smaller h1 text in title section for mobile, because you have horizontal scrollbar.

All best,
M.

3

Yep, looks good and David Gilmour rules :guitar:. I’d just echo @MarekMac on the text visibility/contrast issues. One other suggestion would be to place the image above the text on mobile even if you have to resize it some. Cool tribute page :pineapple:

4

I really appreciate the feedback, thanks for your input :grinning:

5

Thanks for your feedback, I did notice the h1 on mobile but because I spent so much time working out the stacking on mobile for the image, I forgot to tackle the h1 issue :joy: I will work on this soon.

Thanks again for taking the time to look and give me your feedback :grinning:

Paul

6

Hi Paul,

I have rewritten your code in order to simplify it a bit more.

I see that in your code you repeat the same styles several times and I have put them in a single class.

I took the advice that your colleagues have given you and I have also applied them.

Another thing I noticed is that you use units of measure when the value is zero, you can use “0” instead of “0px”.

I also saw that you repeated the @media for the same screen width.

You can use:

@media only screen and (max-width: 600px) {
    .row {
        flex-direction: column;
    }

    #image {
        margin-bottom: 0;
    }
}

Instead of:

@media only screen and (max-width: 600px) {
    .row {
        flex-direction: column;
    }
}

@media only screen and (max-width: 600px) {
    #image {
        margin-bottom: 0;
    }
}

Another thing is that you have:

.row2 {
    display: flex;
    flex-basis: auto;
    flex-wrap: nowrap; <--------

When you use flexbox it has nowrap by default. So it does not necessary.

I hope these details help you to improve much more.

I’m not an expert either, but I think it’s not wrong to share what I’ve already learned with others :slight_smile:

Take a look at this: https://codepen.io/TovarC/full/LBZOzp
7

Hi Jose
Many thanks for looking at my work and taking the time to alter things.
Having said that, I actually still prefer the design of my version :grinning:. Let me explain why…

  • I also tried using a similar red ‘sub-text’, but felt it wasn’t subtle enough for the look I was going for.

  • I also played around with the positioning of the image, but in the end, I decided to center it in the 2nd column for 2 reasons 1. It left equal spacing between the edge of the page and the text in the 1st column 2. It left the background image ‘prism’, in clearer view - and this was what the idea I had before I even started the project.

I know my design choices may not be everyone’s ‘cup of tea’, so forgive me if you think it’s crazy :scream:

I really do appreciate you pointing out the flaws (school-boy errors) in my code though :grinning: I wasn’t confident with it somehow (even though I achieved the look I was aiming for) and I’m always open to hear where I can improve (I have a long way to go)

Many thanks again for your time and input

Paul