Feedback on my first challenge please!

Feedback on my first challenge please!
0

#1

Hello everyone! Just wrapped up my first challenge and I would like as much criticism as I can get. Please don’t be gentle, roast this thing!:sunglasses:

Codepen Link:


#2

:laughing: I won’t roast ya but I do have a suggestion. Maybe break up some of your larger text sections? For example, the text content under SpaceX is kinda massive and when you get down there it’s like a wall of text. I know there’s a lot of info and that’s totally fine to have but it’s hard to read (at least for me) the way it’s currently formatted.

Other then that this looks great! I love what you did with the navigation menu (especially that drop down, super sharp!) and the responsiveness is perfect. I also love the use of the star background with the semi-transparent image of Elon. Well done!!


#3

Hi @jjhugej,

  • Do not use lower levels to decrease heading font size:
 <h2 class="unicornFarts" id="top"> Elon Musk</h2>
    <h5 id="omgLemmeMakeItSmall" class="unicornFarts"> A Tribute</h5>

MDN documentation:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements

Do not use lower levels to decrease heading font size: use the CSS font-size property instead.Avoid skipping heading levels: always start from <h1>, next use <h2> and so on.

http://w3c.github.io/html/sections.html#the-h1-h2-h3-h4-h5-and-h6-elements

h2–h6 elements must not be used to markup subheadings, subtitles, alternative titles and taglines unless intended to be the heading for a new section or subsection. Instead use the markup patterns in the §4.13 Common idioms without dedicated elements section of the specification.

Common Idioms
https://www.w3.org/TR/html5/common-idioms-without-dedicated-elements.html#common-idioms-without-dedicated-elements


  • Target blank vulnerability
<a href="https://en.wikipedia.org/wiki/Elon_Musk" target="_blank">

MDN documentation:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a

Note: When using target, consider adding rel="noopener noreferrer"
to avoid exploitation of the window.opener API.

https://mathiasbynens.github.io/rel-noopener/

TL;DR If window.opener is set, a page can trigger a navigation in the opener regardless of security origin.

https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/

People using target=’_blank’ links usually have no idea about this curious fact:
The page we’re linking to gains partial access to the linking page via the window.opener object.
The newly opened tab can, say, change the window.opener.location to some phishing page. Or execute some JavaScript on the opener-page on your behalf… Users trust the page that is already opened, they won’t get suspicious.

How to fix
Add this to your outgoing links.

rel="noopener"

Update: FF does not support “noopener” so add this.

rel="noopener noreferrer"

Remember, that every time you open a new window via window.open(); you’re also “vulnerable” to this, so always reset the “opener” property

var newWnd = window.open();
newWnd.opener = null;

Cheers and happy coding :slight_smile:


#4

Thanks @dlyons ! I figured that would need some breaking up! I will go back in and update that after I get back from work tonight/tomorow!


#5

Whoa @erretres ! Thank you so much for the detailed response!

I honestly had no idea about the target=_blank portion. This is a good read right here! I originally did it because the project video told me to, and I dd it without looking into it any further.

Regarding the heading levels, good catch! I had no idea I even did that, I was just throwing random header levels in there with the plan to style them to my liking later on. Will keep this in mind going forward.

I can’t thank you enough for your response. Huge amount learned from it!


#6

You are welcome :smiley:

Cheers and happy coding :slightly_smiling_face: