Julia.L-TailTales #49
Conversation
gabriellaberko
left a comment
gabriellaberko
left a comment
There was a problem hiding this comment.
Really great work with the website! Everything works great, looks good on different screen sizes and seems to meet all the assessment criteria + stretch goals. The code looks clean overall, i would maybe only polish it a little bit with more clear naming for some classes and a bit less spacing in the code.
I really like the structure with keeping the files and pictures in separate folders as well, makes it very organized and easy to jump between them. You also had a very neat solution for your interactive hamburger menu! Well done with everything, you should be really proud of your first project!
There was a problem hiding this comment.
HTML code looks really clean and is easy to follow, well done! I only have some minor comments:
- Maybe the script-tag for the Javascript file could be considered to be placed last in the body tag rather than in the head tag
- Only a small preference from me, but it would maybe be even more easier to follow the code by adding more self-explaining class names to the sections and other elements to easier see what they are for (and especially when referencing them the CSS-file) :) (example card-image-2)
|
|
||
|
|
||
|
|
||
| /* Text edits */ |
There was a problem hiding this comment.
Really nice to group the CSS styling like this, e.g. all the text styling in one place!
Everything looks clean and you did a really nice grouping in your CSS overall, with spacing between (you can however minimize the spacing a little bit to just a few empty lines in between to keep the code file shorter!) and some comments above for what it covers!
| @keyframes fadeIn { | ||
| from { opacity: 0; } | ||
| to { opacity: 1; } | ||
| } |
There was a problem hiding this comment.
Not familiar with this. What does @Keyframes do?
There was a problem hiding this comment.
Keyframes is a way to start an animation, you first write keyframes then the animation you want. In my case "fadeIn" since I wanted the text to slowly appear in the hamburgermenu. It can do alot of different animations in your CSS. TLDR: @Keyframes defines how a set of CSS properties evolve over time.
There was a problem hiding this comment.
Smart solution for the interactive hamburger menu! 👌 Much less code to achieve the same thing, compared to what I did myself!
Form/index.html
Outdated
| name="viewport" | ||
| content="width=device-width, initial-scale=1.0" | ||
| > | ||
| <title>my-project</title> |
There was a problem hiding this comment.
Maybe want to change this one to the same title as the main page is using!
Form/index.html
Outdated
| <link | ||
| rel="stylesheet" | ||
| href="style.css" | ||
| > |
There was a problem hiding this comment.
Is there a reason to link the CSS stylesheet for the form again inside the form tag and not just in the head? 👀
There was a problem hiding this comment.
No, that was an experiment I hadn't deleted yet! Thanks for noticing!
Form/index.html
Outdated
| <span>Name</span> | ||
| <input | ||
| type="name" | ||
| name="name" | ||
| id="name" | ||
| autofocus="name" | ||
| required="text" | ||
| > | ||
| </label> | ||
|
|
||
|
|
||
| <label> | ||
| <span>Number</span> | ||
| <input | ||
| type="text" | ||
| name="number" | ||
| id="number" | ||
| required="text" |
There was a problem hiding this comment.
I would look a bit into refactoring this part! required = "text" looks like it might be a typo. And preferably the type = "text" could be changed to type = "number" for the number input field, to make sure that you can actually only submit numbers in that field!
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Make sure your project looks good from 320px width and up. Right now you have some content that are wider than this which makes it look broken on small phones. Building on to this thought, I think it would be nice if the grid was more dynamic or had more levels if that makes sense - i.e. that on middle wide screens you could have three cards in a row (and maybe more cards then). Right now there’s a lot of whitespace to the sides.
PS. Since your hero text is completely centered you could’ve went with the flexbox approach instead of position: absolute.
Nice and clean JS code ⭐
Change request
- Make your project look good on all widths
Form/index.html
Outdated
|
|
||
|
|
||
| <form | ||
| action="http://httpbin.org/anything" |
There was a problem hiding this comment.
Make sure you're using the https method, otherwise most browsers will block this action
3 participants
https://tail-tales.netlify.app/