Portfolio Site Critique


Hi guys, I’d love to hear your feedback on my portfolio page. I’ve been using FCC for a few months now and have loved learning, using this resource :smiley:


Hi Jonathan,

I think that’s an awesome portfolio site.

My only suggestions are to add some left padding so that the menu items and the Skills content are not so close to the edge. And I would also add some padding to each of the form elements and remove the default borders around the submit button.

I think you did a great job overall.

That space background is great! :smiley:
I agree with the padding. Maybe could make the contact form element bigger and add some space between the social media icons and the copyright in the footer …

I like the material design guide is a reference for proportions: https://material.io/components/text-fields/#specs

Thanks guys for the kind words! I’ll work on the spacing in the next few days :smiley:

Ok, I’ve made all of those changes. I’ve also rounded the contact form elements slightly and gave it a light grey background instead of a border.

Is this what you had in mind?

Hi Jonathan, nice portfolio. My two cents:

  • Home: impressive animation. I wish it was a bit slower, it’s a bit overwhelming, especially since it’s supposed to be a star field.

  • Skills: maybe you could change the media queries a bit. On my desktop with a relatively wide window, I see 4 rows of 3 icons with a lot of white space around. The eye has to travel a lot, I’d group them tighter for the user to be able to see them all at a glance. Especially since there are a lot of them. And when I resize to a smaller width, the icons get really small, especially on mobile. I’d say mobile is where you want your skill icons to look good.

  • Portfolio: I don’t like the empty grey squares, it looks as if something didn’t load properly. Like Skills, the images are lost in a sea of white space and they are too small. Those are your projects, you want the user to be impressed enough to click on them. They look like itty-bitty things, they should at least 50% bigger. And check what it looks like on mobile with Chrome devtools, it doesn’t look good. Make it one (maybe two) project wide on mobile, three is too much.

Sorry, maybe that sounds harsh. It’s not meant like that. I think your portfolio is great, it just needs tweaking, especially since you place emphasis on your responsive design skills. You need to show it in your portfolio.

Thanks so much!

Not too harsh at all :slight_smile: The notes on skills and portfolio were things that I wasn’t 100% on anyway but knowing they stick out for someone means that I can refine them further.

As for the background animation, I hadn’t thought of it being too fast before. I guess I should slow it down to the point where it adds movement to the page but without being overwhelming. I’ll tweak it later :slight_smile:

1 Like

I like your portfolio, looks great


@sfiquet the animation i now half the speed, I’ve reduced the white space by bringing up the size of the skills icons and the portfolio pictures have been brought up too.

I’ve decided to go for 2 columns of 3 rows for these and will add the other 2 icons once they are ready (instead of having the dreaded grey squares).

I like the animated header and the fonts that you used. The other sections are clean and simple. The photos from the Portfolio section are a little too small. What I don’t like that much is the contact form.

Thanks! Yes I think I should work on making the portfolio pics bigger.

What is it about the contact form you don’t like?

Is too small. Maybe it will look better with these proportions:

Looks great :ok_hand:

My only critiques would be to:

  • Consider removing the “Home” link in the navbar. It doesn’t appear to serve much purpose.

  • As the site scales down to mobile\smaller screens I noticed some text overlap. I will provide a photo below:



Great choice of color. The green makes your eyes pop.

It looks very nice, no doubt about that. There are a few issues underneath the hood though.

  • Do not set your font sizes in vh (or vw). They should be in em (or rem). The only way to increase the font size on the page is by making the window taller (or wider for the links at the top), and I can only make my browser so tall. This is a huge accessibility issue. You should never try to control the text size that the user can view your page with and your page should respond nicely to bigger text sizes. Sorry to sound so adamant about this but it really is a very big accessibility issue.
  • On the Skills page, add alt="" aria-hidden="true" to all the icon images since you have the text for them below and thus screen readers don’t need to know about them. Also, do not use <br> to space content. That looks like a list to me and should probably be coded as such.
  • On the Portfolio page, your images don’t say what they are. You should probably have at least a header for each image and maybe even a small description of the project. And you need to have text in each of those links. You can visually hide the text if you like. Actually, what I would do is make each header a link to the project. You could also keep the image as a link too but you would want to hide it from screen readers with aria-hidden="true" because you don’t want two links in a row pointing to the same place. Also, for narrow browser widths I would put just one project image on each row so you can have them displayed bigger.
  • On the contact page, your labels can’t be blank, they have to have text in them (for accessibility reasons). The placeholder is not a substitute for a label. In fact, I don’t really think you even need placeholder text for these because everyone knows what you mean by Name, Email, and Message.
  • The hamburger menu icon is placed after the nav links, so when using the keyboard, after clicking the icon, you can’t get to the menu links by tabbing, you have to reverse tab (hold down the shift key while tabbing). This is not the expected behavior, you should be able to tab to the first link in the menu after clicking the icon. Also, the hamburger link needs to have text associated with it (for accessibility reasons, you can visually hide the text if you want).
  • Furthermore, if you are going to make the hamburger menu links take up the entire browser window then you need to either keep the user from tabbing out of the menu or you need to close the menu as soon as they tab out of it. Otherwise they can tab through the rest of your page while the menu is still covering the entire page. Bottom line, the user should always be able to see where the current keyboard focus is.
  • Speaking of keyboard focus, I think you should make the focus indicator a little more obvious. Currently it is very hard to see the indicator around the project images and the Submit button.
  • In the footer, I don’t seem to be able to mouse click on the three icons but yet I can tab through them and click them with a keyboard.
  • Since this is really just one big page I think you should hide those down arrow links from screen readers with aria-hidden="true". And really, you should probably take them out of the tab order with tabindex="-1" as well. They are really just there as a special effect and are not needed to navigate the page. Removing them from the tab order would make it a lot nicer for keyboard users.

I’ll stop there. Sorry if this seems like I’m being overly critical, I really do like the overall look of your page. Accessibility is somewhat of a passion of mine and so that is where I concentrate most of my critique. At a minimum, install some accessibility extensions in your browser and run them against your page. They will find a lot of obvious issues. Make sure you can navigate and use your page 100% with only the keyboard. Any time you use images to represent content/links you need to make sure you have a text alternative for people who can’t see those images. Also, using FF, go to the View -> Zoom menu and activate ‘Zoom Text Only’. Hold down Ctrl and scroll your middle mouse button to make the text bigger (it should get bigger :slight_smile: ). Make sure your page adjusts nicely to bigger font sizes).

You never know who will be looking at/evaluating your page. Imagine if the person evaluating your page for a possible job can only use the keyboard or has a visual impairment.

Thanks everyone! Sorry I’ve been busy on a project but I will look to implement much of these after :smiley:

@bbsmooth - I have a lot to learn about accesability!

One thing in particular I’d like to ask about is the text size as I’ve fallen into the habit of using vh / vw for this.

Can I still have the text size responsive to the size of the page or is this bad practice?

If I were to do that (have responsive text size) then how would I implement it using em / rem? Would I set the body font size as vw / vh and then set each bit of text after that as em / rem?

It is a bad practice as far as accessibility is concerned. The user should be allowed to increase the font size as much as needed. This doesn’t mean that you can never change the font size for different viewport widths. For example, if you have a big h1 header (font-size: 4em) then you could reduce that in narrower viewports. But if you use ‘em’ then you are still allowing the user to control the font size they want to use from their end.

Bottom line, you should be able to increase the text size and your page should handle it gracefully. The responsiveness concerning text size is the ability to handle different sizes.