Profile card component, using HTML and CSS

Community feedback

• 

  Adam Phillips•115

  @adphillips2017Posted over 3 years ago

  Okay so this is my first time reviewing code on here so I'm not exactly an expert, but I figured if I'm going to benefit from others' feedback I had better offer some of my own to the community in return.

  So starting off, just from the design comparison looking at screenshots of the two you got really close. There aren't any glaring issues or anything so good job following the design spec. The only thing that I think needs addressed to more fully capture the design is to get the sizing of the text correct. The text on your solution is a bit larger, which makes it more of the focus of the piece when I think the original design wanted the profile picture to be the main attention grabber. The larger font in the same space also means that there's less negative space which makes it seem a bit more crowded. Like I said, though, great job overall.

  To address your question about where you start with a project, and if you should start with the background, here are my thoughts on that. I like to always start with styling things in the same order they are structured in the DOM. So, body first (font styles, backgrounds, etc.), then major containers, then cards, text etc. etc. I like to go in this order because changing the style of something like the body or container after you're finished styling a card for instance could very well change the way that card looks leading you to need to re-work or refactor the card. CSS styles 'cascade' after all, and it's hard to know just what will be effected further down. That being said, there's no real rule that says you have to go in a certain order, so you can try different approaches and see what allows you to get the most accurate end result, and then try optimizing it to allow you to get more done quickly.

  Taking a dive into your code the first thing that jumps out at me is the commented out section of the initial text you're given. I would advise making sure you clean up any 'dead' code. A good habit to have is to do a search of your working directory to see all the instances of comments ('//' in javascript files, '<!--' in html, etc.) and then clean out any unnecessary comments. If it's old code you're afraid you might need later, commit the code to git with the commented out code in, then remove it and commit again. This way it is forever archived on git if you need to get it back and your current code base gets to be clean.

  The other thing I would recommend is utilizing whitespace more, especially empty lines in between html elements. Your lines 19-34 have no empty lines, which makes it harder for humans to read. This can really improve the readability of your code and make changes / maintenance easier.

  The next thing that I found was what appear to be unused classes in the html: your stat-1, stat-2, and stat-3 classes for example. I don't see them being used in the css, so I would recommend cleaning out code like that which isn't doing anything.

  You also have some comments in your css which I would recommend cleaning up. The ones that are descriptive of the code near it are fine usually, like where you've separated your css variables with /* primary colours */ etc. What I'm talking about is more of the dead code where you've commented out styles that you didn't end up using in the final design. I'd clear them out, and just check the old git commits if you end up needing them again.

  Okay so the only other thing I see that I would change is the name of your css file, which is really not that big of a deal but generally when you only have one style file I think it's common practice to just name it style.css or styles.css. Not a big deal by any means but sometimes it's good to follow common practices to make your code more predictable (and thereby more readable for other people).

  I'm being very picky in my feedback, obviously, but just wanted to point out the things that stood out to me. Your submission is overall well done. Keep up the good work.

  2

  ---

  

  Griselda Knight•280

  @Greeshma2903Posted over 3 years ago

  @adphillips2017 thank you so much for that elaborate feedback!

  I Will definitely work on the readability of my code, and yes I'll try out styling from the background (in DOM order) and see what works best for me! I had this doubt for a very long time, thanks for the advice! 😊

  1