EvelinaLarsson / f1-3-c2p1-colmar-academy

0 stars 0 forks source link

SUMMARY #4

Open blerude opened 6 years ago

blerude commented 6 years ago

Grade: Exceeds Expectations

Amazing job on this project! You've effectively included all the necessary elements and you've made the app responsive to screen size shifting on different devices. Your code is incredibly easy to read and understand thanks to your consistent spacing/indenting and your clear commenting. You're definitely ready to take on larger and more complex projects with those habits down.

Moving forward, be careful when you decide whether to use ids or classes. This article explains a bit more about the difference between the two: https://css-tricks.com/the-difference-between-id-and-class/. You should use 'id' when an element is the only one of its kind, and class when your repeating something over and over (like the courses here). Also, when it comes to CSS, it's great that you're importing your own fonts. Some other resources you might appreciate are Google Fonts (interesting fonts with instructions on how to import them into your code) and Coolors.co (a color palette generator). Again, great job on this project!

EvelinaLarsson commented 6 years ago

Thank you! I'm very pleased with this feedback, as you can imagine! Noted about the class vs id point, will make sure to keep this in mind in the future.

Could you please let me know whether it's common practice to duplicate entire passages of code on web pages, like I've done here between section 2 and 4? My initial aim was to use the same classes across both sections, but quickly realised I needed to separate the styling of the two. I was tempted to rewrite the code to differentiate the class names a bit more, but wasn't sure whether it was worth the extra effort and the risk of missing something.

Would also appreciate if you could take a look at the styling of the CTA for mobile (195-211, 268-314), and let me know why I wasn't able to make it 100% wide as per the wireframes for mobile.

Thanks! Evelina

blerude commented 6 years ago

Hi Evelina,

Glad to help! When it comes to duplicating long passages of code, you want to avoid writing the exact same thing more than once. Try to find a way to abstract your code from section 2 so that it can be used again in section

  1. But if you’ve found that the stylings need to be different between the two sections, then it’s ok.

As for the button, it’s difficult to see exactly why it didn’t stretch to 100% without being able to see the live version of your project (reviewers only have access to your code), but try adding a width styling of 100%. If that doesn’t work, pay attention to the element the button is contained in, as there might be a styling that forces its contents to not stretch its full width.

Hopefully that helps a bit! Thanks!

Ben

On Tue, Nov 7, 2017 at 12:56 PM EvelinaLarsson notifications@github.com wrote:

Thank you! I'm very pleased with this feedback, as you can imagine! Noted about the class vs id point, will make sure to keep this in mind in the future.

Could you please let me know whether it's common practice to duplicate entire passages of code on web pages, like I've done here between section 2 and 4? My initial aim was to use the same classes across both sections, but quickly realised I needed to separate the styling of the two. I was tempted to rewrite the code to differentiate the class names a bit more, but wasn't sure whether it was worth the extra effort and the risk of missing something.

Would also appreciate if you could take a look at the styling of the CTA for mobile (195-211, 268-314), and let me know why I wasn't able to make it 100% wide as per the wireframes.

Thanks! Evelina

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/EvelinaLarsson/f1-3-c2p1-colmar-academy/issues/4#issuecomment-342460803, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQmdJnng827-6qkkrdPC8kieHjMvk4bks5s0EV8gaJpZM4QUcvx .