This is my first attempt at building a website and publishing on Github! I didn’t think I was going to enjoy web dev, but so far it seems to be pretty fun. I’m excited for more learning!! I am open to any and all feedback, as I made some decisions in this project that I was unsure about.
This is my Dasmoto’s Arts and Crafts Project which I came across on the Build a Website Skillpath. I made the banner slightly larger, although I don’t think the text is vertically centered within the background-image. Maybe it’s just my eyes.
My code can be found here on my Github
1 Like
Well done on your first website - and publishing it on GitHub (which makes it so much easier to code review!).
It’s looking good, and you hit almost all the requirements for the project. Tips/feedback:
- It’s a good idea to share a link to the course page and/or the design spec so reviewers understand your project. In this case I have done the project, but through a different path, and there may be some minor differences. You’re the second person I’ve seen mention “vertically centered” text, however the spec I have access to says “centered horizontal”…
- Nice clear code, you’ve structured and written things well
- While the design spec didn’t state everything explicitly, the goal is to try to replicate what is in the design spec. For this reason, try to get the banner the same rather than larger, and the h1 title to be on one line when the browser is at full screensize.
- There’s redundant code for the blue price text, where it has been coded “bold” both in the css stylesheet and inline in the html. I would recommend keeping all the css in the stylesheet (hypothetically, if design changes were made, you could easily change them in the stylesheet and not have to hardcode many instances in the html).
- It may not have been covered in your skill path, but where possible use semantic HTML instead of generic html… currently each of the sections is wrapped in a
<div> where the semantic HTML option would be section tags instead.
- It’s a good idea to include a backup font in case the first font doesn’t load e.g.
font-family: Helvetica, Arial, sans-serif;
- Extra marks - your design doesn’t break when it is viewed on a smaller screen, like mobile.
Great job!!!
I’d also highly recommend doing a code review for someone else… examining your code against another person’s will help you see more… this code review made me realise that my own design breaks on smaller/mobile screens - so I will have to look into fixing that!
Let me know if anything I’ve written above isn’t clear, or you want to discuss. It’s also fine to disagree with the above suggestions - it is your project, and there is nothing wrong with what you have completed.
3 Likes
Thank you so much for your feedback! It is greatly appreciated. I do have one question: I was unsure how to style the blue price text separately in CSS yet within the same <p> element. Would I have been able to style the <span> element? Should I give each <span> a class name?
I don’t believe the course page mentioned anything about “vertically centered” text, but I didn’t look vertically centered to me and I thought it might look better that way.
Thank you, I will definitely link the project specs in my future posts, I didn’t think about my readers fumbling through the site to find it.
Yes, any of these options would work.
- If the only time
<span> is used is for the blue text, you can declare the CSS style rule on <span>
- Best practice would be to set a class name (e.g. class=“price”) in the
<span> and set the CSS rule by class.
- fyi you could also set the style like:
p span { color: blue} to select all span tags within a paragraph tag.
Thanks for giving me an update, it’s always nice to know if people have found feedback helpful, and to receive follow up clarifying questions 
1 Like
@lucitemple Right on! Thank you very much!
You’re welcome for the update. I’m in college right now, and one thing that’s been great about remote learning is that they have done a pretty well job of teaching us how to communicate well online. 
1 Like