Beta Portfolio Project - Jan Cohort

Code Feedback
1

This was largely just my portfolio that I had finished just before joining up with FCC that I’ve repurposed for the beta curriculum.

I added css variables so I could switch the theme around easily, right now it’s a grey scale which I always kind of default to. It’s blue theme live which I do think looks a little better.

I cleaned up a little bit of the CSS, I was just learning grid when I created the site and it’s a little sloppy. The site could probably use a major overhaul but I wanted to wait until I am comfortable with React and less/sass and showcase them with a redesign. I kind of always feel like I’m waiting to learn the next thing before I start delving into projects though . :confused:

Here is a link to my beta portfolio.

8 Likes
2

That’s top quality! I see you’ve done a lot of work in the CSS. It looks brilliant and works well.
I had a look at some of your projects which have great style too.
I would think you would get work as a front end dev soon.
I would be happy to work with you on a project. :+1:

2 Likes
3

Thanks for the words of encouragement!

Frankly I’ve been a little discouraged lately so it’s definitely a welcome pat on the back.

1 Like
4

I like the blue theme better then the grey one.

However the header image in the grey one is better, cleaner, and more professional then the header in the blue one.

I never understand why people do this

class WebDeveloper { 
     constructor( name) { 
         this. name = name; 
    } 
}

const me = new WebDeveloper( 'Kevin Reynolds')

If you’ve created that portfolio you’ve already proven that you can code, do you need to do it in the homepage.

I really like the slideshow.

6

That’s fair, I guess I just thought it was a cute (maybe cute isn’t the right word but it’s all that’s coming to mind) or original twist on just putting my name and position on the site, I didn’t really think it’d be taken as an endorsement of my skills.

But yeah I do see the appeal in the minimalist style of the latter. Maybe I’ll push that to live before I start my next round of applications.

7

Of the 2 styles I think I like the blue style. I actually like that code over the image.
Also the image doesn’t take so much space as the grey theme. I can see your about section straight away.
I also prefer the font used in the blue theme.
Both excellent though.

1 Like
8

Hi, looks great. One small thing, I think a bit more space between the footer and the main section. It looks a little cramped. Other than that great work!

1 Like
9

Very nice page man, I hope to be as good as you are soon! :grinning:

If you don’t mind a suggestion, don’t you think it would be better if your menu merged in mobile? I have messed up a little bit with the size of the screen, and the menu gets a little clunky on smaller sizes. But maybe that’s just my browser.

Other than that I think this is very very good!

1 Like
10

Yeah, I see what you mean. Thanks for the tip!

11

Hmm I’m not sure what you mean by merged, is it the same on the live and beta site? Could I ask what browser you’re using?? I think the beta one could use a little more spacing but it should be responsive. I used CSS grid so maybe that’s the culprit :confused:

12

Like this example from the exercise:

Open it up in a new window and change the size. Notice how the menu merge into three horizontal bars? That’s what I meant.

When I do the same thing with yours they stack together and it is a little bit clunky. I’m using Firefox.

1 Like
13

Looks great. I prefer the beta one… looks cleaner and more professional IMO.

14

Interesting, yeah I just looked in Firefox and it was acting strange.

I figured out what was causing it, but I’m not sure why, maybe someone more knowledgeable can enlighten me.

I had max-width: 1024px; in my navbar class so that on widescreens the nav icons wouldn’t be all over the place and for some reason when my grid-columns are changed from set values 4fr 1fr 1fr 1fr 1fr to a more responsive repeat(auto-fill, minmax(200px, 1fr)) it was setting the navbar to a static 1000px width, I believe 200px for each element in my nav.

I just moved max-width into my media query and that seems to have worked but I’m still curious why it acted like that.

1 Like
15

@KevinReynolds Great work here! I like that you’re using CSS Grid and CSS Custom Properties! :fireworks: Nice job with the CSS animations and transitions. Here are some things I noticed:

  • Accessibility Warnings: When I plug your code into the WAVE and aXe accessibility tools, I get some warnings. They include skipping header levels, color contrast issues, etc…

  • Document Outline: It’s my understanding that it’s still best to only use one h1 element per page. I think this has something to do with browsers not implementing the document outline algorithm that takes into account the new HTML5 sectioning elements. Here’s MDN’s statement about this problem at the top of their Using HTML sections and outlines page:

    Important: There are currently no known implementations of the outline algorithm in graphical browsers or assistive technology user agents, although the algorithm is implemented in other software such as conformance checkers. Therefore the outline algorithm cannot be relied upon to convey document structure to users. Authors are advised to use heading rank (h1-h6) to convey document structure.

  • Missing Heading in About Me Section: This goes along with the above point, but there’s a little more context to add in relation to sectioning elements. According to the specs:

    Each section should be identified, typically by including a heading (h1-h6 element) as a child of the section element.

    w3.org has a great explanation of how missed headings affect the experience of screen reader users:

    The heading hierarchy is the best way to understand the relationship between different sections of content. If the hierarchy is logical (cascades without skipping levels), then those relationships are easy to determine.

    Headings also assist with content location. Moving between headings that have a logical hierarchy enables you to drill down into the page content, narrowing down the part of the page you need to examine in detail to find the content you’re looking for.

    If the page doesn’t have a logical heading hierarchy, then neither of these things is possible. This significantly reduces the UX from the point of view of a blind person.

    Léonie Watson

  • CSS Linting: I noticed some warnings you may be interested in when I copy/paste the CSS into CSSLint.net. You have a lot of CSS, so you’ll have to wade through a lot of warnings related to CSS Grid and CSS Custom Properties (the linter hasn’t been updated to account for those newer features yet). However, some of them are more relevant and you may find the reasoning behind these warnings interesting.

Those are the main issues I notice, relatively nitpicky as they are. Hopefully it’s helpful. Other than those issues, and like others have said above, you’ve done really great work here! :sunny:

2 Likes
16

@camper Accessibility is definitely something I need to look into more, I actually did look over the my linted css and dismissed it all as prefix/css-grid/variables that weren’t known by the linter yet. Looking over them more carefully you’re right there are a couple of things I didn’t notice hidden in there. Thanks for the in depth review! I really appreciate you taking the time!

1 Like