@MacstorkSubmitted almost 2 years ago
This is my first project. I did it using flexbox and grid properties. If you have any feedback or suggestions what should be improved feel free to comment. Thanks a lot !
Jordan Phillips
@JordanPhillips-hubAll comments
- @JordanPhillips-hubPosted almost 2 years ago
Hey Maciej I like your solution a couple of things I would consider changing is there should only be one <h1> tag on a page I would change this and use a span like this to avoid using two <h1> tags <h1 header__title> Reliable, efficient delivery <br/> <span header__title--dark>Powered by Technology</span>
</h1> Also, consider changing your <h3> tags to <h2> tags and adjusting the font size with CSS as heading levels should increase by one for accessibility reasons.0 - @yourpaldiggySubmitted almost 3 years ago
This is the first CSS I've done in about 5 months. My Team Treehouse Tech Degree courses are focused on JS, and frameworks so I don't use CSS or HTML all that much and I'm trying to get better at this. Please let me know anything I could do better, thanks!
@JordanPhillips-hubPosted almost 3 years agoHey Anthony try using a <main> tag instead of a div on your bg this will give your page a main landmark as you have none in your HTML, also try using a h1 instead of an h4 and just adjust the font size to what you want your first header should be an h1 for accessibility reasons..
Marked as helpful1 - @AmmarCodeSubmitted almost 3 years ago
I'd appreciate if you can take a look at my solution and give me some feedback. Constructive criticism is what I'm looking for! don't hesitate to be brutally honest :)
@JordanPhillips-hubPosted almost 3 years agohey Ammar the card looks good although I am not sure if you are getting HTML tags and classes mixed up you should remove the <container> and <card> as these are not tags try wrapping them with a <div> </div> and giving them a class like this <div class="card"></div>, <div class="container"></div>, if you wished to you could remove those altogether as you already have your <main> tag as a wrapper and do not need the other two.. hope this helps you. Here is a good source about HTML tags and when to use them.
1 - @herreranicolasSubmitted almost 3 years ago
Hi everyone!
I tried to apply all the corrections, thanks to your feedback.
Let me know if there is anything I can do better.
Thanks in advance!
@JordanPhillips-hubPosted almost 3 years agoLooks good.. one other thing you could do on this is to switch the h2 to a h1 and change the font size to font-size: 1.375rem; this will get rid of the rest of your accessibility issues. Generally, it is a best practice to ensure that the beginning of a page's main content starts with a h1 element, and also to ensure that the page contains only one h1 element.
Marked as helpful1 - @herreranicolasSubmitted almost 3 years ago
This is my first challenge.
What do you think I can do better?
Feel free to give me your feedback!
Thanks in advance!
@JordanPhillips-hubPosted almost 3 years agoTry putting the blueish grey background colour on your body instead of the .container this should fill make the whole page that colour and get rid of the white patch at the bottom.. Looks good though.
Marked as helpful0 - @95FredoSubmitted almost 3 years ago
Fun project but got stuck on bringing the social-icons down to the end of the page, I tried using align-items:flex-end but it didn't work. So I ended up using transform: translateY(40px). Any feedback on my code is much appreciated.
@JordanPhillips-hubPosted almost 3 years agoAlign items controls the cross axis when flex direction is row that is up and down, justify content controls the main axis witch is left and right, try using justify content: flex-end.. the axis will be reversed when flex direction is column cross axis left and right main axis up and down.
1
