ackleyd1 / codelab-css-transitions-animations-transforms

https://github.com
0 stars 0 forks source link

Code Lab Feedback #1

Open ackleyd1 opened 8 years ago

ackleyd1 commented 8 years ago

Hey @chrisvfritz, can you take a look at this? It's hosted here and meets the following project criteria:

chrisvfritz commented 8 years ago

Looking very good for the most part! I just see a few issues I want to address (and the first two are likely new information):

img tags should have a non-empty alt attribute

Although the W3C validator fails to correctly detect empty alt attributes, they should have text in them for valid img tags in HTML. The purpose of the alt tag is to have something useful show up on the page if the image doesn't load. It's also very useful for accessibility. If someone who's blind is visiting your page, for example, a screen reader has no way of describing what the image is without an alt tag.

The / in self-closing tags

Self-closing elements (or sometimes called "void elements"), such as <img /> and <br />, do not actually require the / in HTML5. They can instead be written as simply <img> and <br>. Using the /s is not strictly wrong. And by that, I mean it's not invalid HTML5. But I feel compelled to discourge them, simply because many companies' style guides, like GitHub's for example, completely bans them in their code.

Logo animation should happen on hover

I know it sounds like nitpicking, but animations can sometimes work a bit differently on hover, so want to make sure you gain that experience.

Inconsistent indentation

The vast majority of the project looks fantastic, but there is some inconsistent indentation in styles.css. Throughout most of your project, you're using 4-space indentation, but then you use 2-space indentation for the keyframe.

ackleyd1 commented 8 years ago

There ya go, fixed suggestions. the keyframes were copied from another source and had that formatting :0 and the img / came from tab completion! cant believe it. why wouldnt they get rid of them.

chrisvfritz commented 8 years ago

Excellent! :shipit: I don't always like what Atom's built-in tab-completions give you. I often define my own instead. :smiley: