Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kayla Ecker - Caret #42

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Kayla Ecker - Caret #42

wants to merge 6 commits into from

Conversation

kaylaecker
Copy link

No description provided.

@CheezItMan
Copy link

Category Feedback
Git usage More commits would be better. The messages aren't so clear what has been done.
Layout Nice work getting the fixed header. Also the post columns are lined up 2 across instead of three across. That can be hard without a grid layout. Also the footer links are not aligned with the text, something needs to be floated.
Images The image under the header isn't there, instead there's a page background. Also "Everybody wants to be a cat!" should involve an image not just text.
Text Background images can be problematic especially when there's a chance for light colored text on a light background or dark text on a dark background. This happens with the clouds and text in your page. It makes the text harder to read.
Code Readability The comments help with the readability
DRY CSS There are some minor things that could be refactored, like you've set several items with width 40% and that could be grouped together. Also some colors could be grouped together.
Overall Definitely some more work needed to be done. You have the bulk of the layout in place, but lots of detail remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants