@alexcarmonadev
Abdullah Nassif
@AbNassifAll comments
- @AbNassif
Love your work man! Would you mind if I asked how'd you manage to size the elements just like the design?
- @yummycoffee
- @effycoco@AbNassif
Great job man!
- @suansen@AbNassif
Hello again man!
Hope you remember our interaction on your first project.
Really love how much you have grown in such a short amount of time, Your code looks really clean!
You can focus on mobile responsivity now.
Great job, Keep up the good work.(Thank you for inspiring all of us with your dedication!)
- @suansen@AbNassif
Hey man, would advise you to go watch at least 2 walkthroughs of HTML and CSS website on youtube, code along then come back here and practice.
Don't give up!
- @MichaelEdoigiawerie@AbNassif
Amazing job!
- @abdo-kotb@AbNassif
Greetings,
This looks quite amazing, you're really skilled. I learned quite a lot from you while reviewing your code.
Frankly, the only problem you're dealing with is that you have to scroll to view the whole card, and the card could be perfectly aligned in the center without the need of scrolling(This could cause problems in real-life cases as whitespace is really important).
To fix this, add a width of 100% and a height of 100% to the head and body( I saw that you tried to align the content in the center when you applied the align-items property in the body, but the reason it didn't work is that the body wasn't taking the full space of the page and those properties above will fix that).
But this will cause problems with the media query when the screen gets smaller(The upper part of the card won't be visible as it's gonna be shoved on top of the page) To fix this you can add (height:100%) to the .main in the 767px media query you wrote, and it will take it's children's height accordingly and wrap around it responsively.
Ps. Great job on adding animations to the attributes, it took me by surprise :).
Overall, great work! keep it up.
Marked as helpful - @Yulia182@AbNassif
Besides the top right button and the footer having a different color, this looks quite good!
Good job, keep it up.
- @greardrear@AbNassif
Hey man, A problem I'm seeing is that the container isn't being aligned perfectly in the center, and it's being pushed out of the screen( I have to scroll horizontally then go down vertically to view the cards).
And after some inspection, I've found out that both your HTML and body tags have a fixed height of about 37px. Now usually the container shouldn't be overflowing from the content, But you gave it a fixed height and width along with some fixed margin inside it which forced the parent elements to remain small while the child(.container) has to be bigger which caused it to leave it's parent's confines to abide by the fixed properties.
Adding fixed pixels isn't always optimal as it wouldn't be responsive across all devices like you hope it would.
Things you can change to align the items perfectly:
Add html, body,.container,.wrap{ margin:0; padding:0; width:100%; height:100%; }
then add align-items: center to the div with the class of wrap.(Since you're using flexbox, you don't need to use margin: auto as you can already use align-items to align the elements vertically).
now the cards will somewhat work on all devices with big screens, but as it reaches around 900 px, the items will stretch to the whole width( You can then fix it with media queries by either giving the wrap a percentage width, or some padding).
Important: Most coders usually include this default code in their projects . *(star means select all tags){ margin:0;(will remove the margin as some elements have them by default like the body for example) padding:0;(same reason as the margin); box-sizing:border-box;(people prefer this as it makes adding padding easier, look up how it helps on w3schools, their explanation is better) }
Ps. next time just use one container for the cards (No need for the .wrap element)as that would be enough to manipulate the cards using Flexbox or CSSGrid.
Great work, keep it up and you'll be doing amazingly in no time.
Marked as helpful - @vijayarajan2244@AbNassif
The Fourth section ("Div.row lg-row4") is taking too much space along with some padding. Now I didn't investigate the reason thoroughly but it's most likely either an inner child being assigned with too much space, or the parent itself.
Great job and good luck!
Marked as helpful - @carlwicker
- @Naimul11@AbNassif
It's okay, spend more time doing walkthrough's on youtube for now (and code along). Finish a couple of websites then come back here and test your skills
Marked as helpful - @rhollings@AbNassif
First of all, really love your attitude towards knowing you made mistakes. We all do, and they are essential in making us the best at what we do.
1- Love that you took the initiative in making the navbar fixed to the top as someone is scrolling. But I can't see the links once I've scrolled, so consider adding a darker background once a scroll is initiated. (Google ways you can do that, but I think you most probably need javascript for that)
2-The nav bar isn't responsive because you added padding of 45rem to the right for the li.left-nav. That is a big no-no in the css standards, as it won't scale well on any other device other than one's similar to your screen.
To fixed that, remove the logo from the ul all together and add it to a separate div.tag (It should be the ul's sibling instead of it's child). Then a super easy way would be to add another div tag in between the logo and links and give it a margin of auto. That way it'll take up the whole space of the nav bar and align them perfectly across all screens.
(There are plenty of other ways, but this is the easiest of them all)
3-Always and always fix the responsivity of the section before creating another one, I did that once and it ended really badly for the project I was working on. Learn media queries and read about responsive units(Vh,VW,%) and how you can use them to reduce the code required for mobile responsiveness.
4- To fix the orange missing as the viewport changes, just change the "background-position" property to "bottom" in media query.
5- You forgot to add the milk image above the footer. Just add a display of flex, then give all of them a specified width to fit the viewport, and add flex-wrap to break the down vertically as the viewport get's smaller. You could then use media queries to increase their width as each image get less competition for the row it's in.
Honestly couldn't figure out the reason behind the whitespace between the images in the center, and why they're taking 150% of the screen size(having to scroll horizontally), But It's so messy and it would be counterproductive to spend your time fixing it now. So just pat yourself on the back as this challenge isn't as easy as it seems, and remember to break everything down next time and make sure that each part is mobile responsive before moving on to building the next part.
Great job!
Marked as helpful - @abhayxkumar@AbNassif
Although it looks super great(honestly it's like a clean-cut copy of the design!), the problem is your design is too big. Now size shouldn't matter in most cases, but I have to scroll down to view the rest of the card, and in real-life cases, this could cause some problems.
Other than that, amazing work!
- @vijayarajan2244@AbNassif
Your culprit for the horizontal overflow is the button in the Navigation bar that contains the text "Try it for free". A cute way of solving it would be to add an (overflow-x:hidden") to the parent and forget it exists. But in all seriousness, figure out what class in the desktop viewport caused the button to have all these margins in the mobile view.
Other than that, It looks super good. Great job!
Marked as helpful - @hemantsirsat@AbNassif
A little problem you might be facing with your code performance-wise is that you included both images (desktop sizing and mobile sizing) in the image container. Now as smart as that is(Seriously never thought about that one before), It could affect the site's speed in the long run as you're loading both images in the HTML.
How to fix that: 1-CSS Approach(Easier and more effective): A good practice would be to not add an image inside the container, but add a background image to the image container itself with css, then change it with media queries(Media queries are essential for the responsivity of the website. This will teach you about it in 7 mins! https://www.youtube.com/watch?v=yU7jJ3NbPdA);
2-Javascript approach(Good to know how powerful this language is when you want to learn it later on)
Javascript can change the src of the image while listening to the viewport!
Design Problems:
1-To fix the image from overflowing from the page on the desktop, just add a height of 100vh( Means take 100% of the viewport no matter what device is used) and don't assign a width to it nor the parent. This will tell the image to apply width automatically with respect to the height of the image.
2- Aligning content with fixed margins can only be used for certain cases, learn flexbox, It's super easy and it was created to align items in the easiest way imaginable across all devices(Flexbox with media query will drastically improve your design game)
If I was you now, I would take a couple of days off of front-end mentor, learn about media queries(would honestly take you a couple of mins to understand how to use it), Then do a flexbox crash course created by "Travesty media". Then I would come back here and apply those concepts to see how much they make things easier.
Other than that, great job, you seem to understand the fundamentals pretty well, Can't wait to see more of your work!
Marked as helpful - @developedBySwan@AbNassif
1- In the Egg Image section, you can actually just add a width of 50% without a height property. These images are specifically designed to take up the spaces in the design. So giving it a width of 50%, would mean "assign the height of the image meanwhile respect the width of 50% of the image",.
2- Really loved the initiative you took with the fixed nav header, but the background should activate faster as the link text is overlapping with the main header.
3- The background of the footer is grey instead of the greenish color displayed in the design, and it's not your fault as it wasn't displayed in the "Style-Guide", but in these cases (and real-world as well), you could just use a website like "https://imagecolorpicker.com/en" to get the color displayed in the design.
4-You forgot to add a hover effect to the button in the nav using :hover in css and including a transition for it to smoothly change.
5-The nav bar is opening on it's own when the viewport get's small enough.
Remark: You should practice responsive units to reduce code, and get comfortable with flexbox and css grid offered by CSS. Learning a Css framework without mastering it first will really frustrate you and prolong the journey ( Don't ask me how I know that).
Last but not least, great job! You did quite well, don't forget to keep on practicing and hit me up if you have any questions(Would love to have a coding buddy!).
Marked as helpful - @Huseyn-SomeGuy@AbNassif
You could center the content by adding display:flex; Justify-content:center; align-items:center; To the body element.
But don't forget to add a height and width of 100% so the body element takes up the whole space and align the items correctly.
You could do the same thing to the wrapper instead if you want to.
Other than that, great work, keep it up!
