P
@harshitkumarpatel28
P
Sven Notermans
@Sven-27All comments
- P@Sven-27
Hi,
I just looked at your live site and i see that somethings can use attation.
- Your spacing is of in some places.
- The large image is strange. If i slide the page to smaller or larger width it seems it is not always in place and in de lg range the top corners dont have a radius.
- if i make de screen smaller then the text of the button is not entirely visible.
For your code.
- Your html looks good.
- Try to use variables in your css if you have to change anything you only have to do it in one place instead of looking after the entire file.
- Use a more flexible value like rem instead of px. This helps the responsiveness.
- In your javascript make variables in the top of your file. Then you dont have to use document.getElementById constantly.
- You are doing everything in one funtion. It is better to create new function for every single task. You can call the functions in other functions if needed. But debugging will be easier and it helps the readability.
I leave it at this. I hope you can do something with this
Regards Sven
- P@Sven-27What are you most proud of, and what would you do differently next time?
No comments
What challenges did you encounter, and how did you overcome them?No comments
What specific areas of your project would you like help with?All feedback is welcome
P@Sven-27Hi,
I see what you mean. But then the social container doesn't overflow the container. It will be cut off.
Haven't found a solution for that yet.
- P@joeabrahamianWhat are you most proud of, and what would you do differently next time?
I am getting better at using media queries and making different screen sizes adapt to my web page.
What challenges did you encounter, and how did you overcome them?Struggled to overcome them but I didn't know how to make an absolutely positioned element adapt to different screen sizes. I'm guessing that you have to have several media queries in place to keep an absolutely positioned element in one spot. I only had one for 400px for mobile.
What specific areas of your project would you like help with?More overall practice. I am excited for Javascript
P@Sven-27Hi unfortunatly i can not view your code because i am getting an error. But based on looking on the page itself. I see that the responsiveness is not perfect. Especcially for the midsized screens. This is something that needs attention.
And the second thing is the placing of your social icons component is not how it should be. For midsize screens and larger screens it should stick to the right like in the designs. If you slide the size of the screens it does not stay in place. And for mobile it should fall over the author section. You can achieve that with position: absolute; inset: 0;
Hope you can use this.
Marked as helpful - P@Sven-27What are you most proud of, and what would you do differently next time?
The responsiveness is good. But de background image in the footer is not entirly responsive. For the screensizes given in figma it is good. Next time in a real project i would pay attention to the image so it is more scalable.
What challenges did you encounter, and how did you overcome them?Because the background images for the footer had fixed sizes i wasnt able to get it entirly responsive
What specific areas of your project would you like help with?somebody has tips to use images with fixed sizes and make them scalable?
P@Sven-27Hi,
Thanks for your feedback. Your right i didn't think of that. But i will correct that.
- P@mkerr-githubWhat are you most proud of, and what would you do differently next time?
Getting the three different layouts on mobile, tablet and desktop to closely match the design. Next time I would like to do a cleaner set of sections and to make the margining and padding easier.
What challenges did you encounter, and how did you overcome them?This was my first full page layout on Frontend Mentor.
This landing page was harder than it looked. I had not used background images much before, so had to do bit of trial and error on those.
I also found switching between the different number of hero section images on mobile/tablet to desktop tricky, but overcame it by using background images for the hero section on the desktop layout.
What specific areas of your project would you like help with?Nothing specific, but always looking for suggestions on how to improve! Especially if there was an easier/more efficient way to do things. 💪
P@Sven-27Hi,
Your html looks pretty good. So props for that. But i would use a tag instead of button. Because the a tag has a download attribute.
Your css looks also pretty good. So keep it up.
- P@underhrWhat are you most proud of, and what would you do differently next time?
I'm most proud of using the CSS grid. This was my first time really using it, & I'd been using flex box much more recently.
What challenges did you encounter, and how did you overcome them?I struggled a little bit with getting the grid sizing & layout right. The columns weren't the right width & for most of the boxes, the text wasn't filling up the box. I easily solved this issue by removing a line of css, unfortunately I have no idea what it was i removed, I can't remember. But it works now!
What specific areas of your project would you like help with?As usual, anything you think I should know or anything I could improve on.
P@Sven-27Hi, For the first time using it you did a really good job.
By looking at the live link i see that the responsiveness is not a 100%. If i slide the screen to a smaller width then i see that the grid moves out of the screen.
HTML:
- I notice you use pretty much only div elements aside from image and span. Use more semantic elements like main, article, section, h1, h2, h3. It greatly improves the structure and readability.
- The element with class title you used p element, instead you could use a heading element like h1 or h2. Other then that the html looks good.
CSS:
- You dont need a max-width in the container. you can make the grid-template columns more responsive by doing the following. grid-template-columns: repeat(autofill, minmax(16rem, 1fr)). This way you already have a completly responsive grid. But because some columns and rows have a span of 2. You need to make a media query with a min-width of 1025px. Then you can do the following. grid-template-columns: repeat(4, minmax(14rem, 16rem)); This way it is fully responsive.
- I also see in your media query that you change the grid-template-columns but that you have a display block. If you have a grid-template-columns you need to use display: grid. Otherwise the grid-template-columns does not work and is it redundant.
Hope you can do something with this feedback.
Grtz Sven
Marked as helpful - @SifanfisehaP@Sven-27
HTML:
- I would put the section description part in a header. That way you can keep the cards section in the main element and you can erase the div with class cards. This one is not needed then anymore.
- You can change the section element with class section-card into a main.
- I would turn the divs with class card into an article element and remove the divs that is wrapping the text and the image. This is not needed. And your dom is much cleaner.
CSS:
-
Your responsiveness is not good yet for the mobile version. The cards are not centered and the text above your cards isn't getting smaller in width and font size. I would advise you to take a look at the clamp function of css. With this you can make 95% of the responsiveness without media queries.
-
for text elements you can use max-width: 60ch. for example. Then that element won't show more dan 60 characters on one line. This way you adding more responsiveness to your text elements.
-
Put only things in your css what you really need to keep your css file as short as possible.
For example:
/* 3. Add accessible line-height / line-height: 1.5; / 4. Improve text rendering */ -webkit-font-smoothing: antialiased; }
/* 5. Improve media defaults */ img, picture, video, canvas, svg { display: block; max-width: 100%; }
/* 6. Inherit fonts for form controls */ input, button, textarea, select { font: inherit; }
/* 7. Avoid text overflows */ p, h1, h2, h3, h4, h5, h6 { overflow-wrap: break-word; }
/* 8. Improve line wrapping */ p { text-wrap: pretty; } h1, h2, h3, h4, h5, h6 { text-wrap: balance; }
This is not needed.
And if you reset the margin don't forget to reset the padding to.
These are the biggest things i saw and i hope this feedback helps you.
Regards Sven
- @JosiasSandovalP@Sven-27
Hi, From what i can see the fonts have the wrong font families. And the spacing is a bit of.
unfortunately i cant open your code. Because if i click on view code i get an error. So i cant give feedback on that.
- @danielnero-botP@Sven-27
Hi,
Your HTML looks very good. Almost like mine. Except for the nutritions part at the bottom it would be better to use table tere instead of divs and spans.
Styling: Your image does not show. I am missing your assets folder in your repository. Your font-family is different. That is either because you are missing the assets folder with your fonts. I also so no import from google fonts.
Your using display grid in places were it is not needed. Display flex would be enough.
Look over your paddings and margins to. With the ingredients section for example.
With list you don't need to define list-style disc. This is already default value.
These are the main things that i see.
Hope this helps you. If you have questions just ask.
- P@Sven-27What are you most proud of, and what would you do differently next time?
I wouldn't do much different.
What challenges did you encounter, and how did you overcome them?It was a little bit of searching how to set the navigation by keyboard
What specific areas of your project would you like help with?All comments are welcome.
P@Sven-27Hi,
Thanks for your feedback. I get what you mean with the divs. But i always learned to keep the dom as clean as possible. And in this case it is short and not needed. As for the widths i used the widths that are in the design in figma. But i can check to be sure.
As for the comments you're right i could use a little bit more of them.
- @ahmad-nmP@Sven-27
Hi,
I am looking at your code and a couple of things could be improved.
HTML:
- No use of semantic elements. Study up on this.
- Image is not appearing. Your src path is wrong. Remove anything before /Images and just put a . there.
- You used button elements. Instead it is better to use anchor element. Then it is possible to link them to your social media pages. And there is no tabindex. So people kan navigate with keyboard properly.
CSS:
- The spacing between the elements are not as they should be. Look at the figma. If you have trouble with it, just ask me what you want to know and i will help you.
- In figma there is a file with the colors you and so on. Try to create variables in your css. That way if you have change something in the project you only have to do it in one place instead of everywere you put it.
- Your text is not responsive. And the width set for tablet. Also take a look at your media queries.
If you have questions contact me at discord and i will help you.
Hope this is helpful to you.
Kind regards Sven
Marked as helpful - P@Sven-27What are you most proud of, and what would you do differently next time?
It was already more easy to work with figma. I now understand how it is made. It was nice to learn how to make the fonts responsive with clamp instead of media queries.
What challenges did you encounter, and how did you overcome them?No big challenge yet.
What specific areas of your project would you like help with?Just all feedback is welcome. Improving is what it is about.
P@Sven-27Thank you. Nice to hear.
- P@davelilleystoneWhat challenges did you encounter, and how did you overcome them?
Choosing the correct semantic HTML can be quite a challenge, I assume that that will become easier with more experience.
I had a little difficulty implementing the changing box shadow on hover, in the end I found out about the :has() pseudo class that allows you to style a parent based on a child's state.
P@Sven-27Hi,
Your code looks good. But in regular css is it also possible now to structure your css like the html. Like you can do in SCSS. The use of semantic elements is good. So keep on the good work.
Kind Regards,
Sven
- @ShadowCoder040What are you most proud of, and what would you do differently next time?
I am happy that I got more comfortable with CSS and finding the correct solutions to position each element correctly, almost on the first try.
What challenges did you encounter, and how did you overcome them?As usual the media queries are still a bit of mystery to me, but the more I practice the better I understand.
P@Sven-27Hi,
First of all it looks good wath you did. Couple suggestions i have.
IN your html you use mainly div. Try to use semantic elements as much as possible. Div is used when no other element is suited. Like <div class="footer"> You can do <footer> instead.So it is a good thing to learn more about them. Otherwise it looks good.
CSS: I see that you have imported the fonts directly. Since they are already in de asset folder it proberly was the meaning to use them instead of importing.Look at 'font face' to use local fonts in your project. The use of variables is perfect.
You didn't make the fonts responsive. Try to google how to make the fonts responsive without media query. That was part of the assignment. Try to structure your CSS the same as your html. I will give you my github link so you can see what i mean.
Take a look at your hover on the h2. It does not turn yellow.
Hope you can use this feedback and if you have questions you can always ask me either here or on discord.
My github is https://github.com/Sven-27/blog-preview-card
Regards Sven
Marked as helpful - P@Sven-27What are you most proud of, and what would you do differently next time?
This is just a good simple project to get used to frontend mentor and how it all works.
What challenges did you encounter, and how did you overcome them?I know html and css. So i did not ran into problems. With this projects i was more focused on figma and how that works and how to get engaged with frontend mentor rather then the project.
What specific areas of your project would you like help with?N/A
P@Sven-27Hi, thanks for the feedback. With this assignment it was more important to figure out how the figma files were build and were i could find the right info i needed.
But i will keep it in mind for the next one.
- @darbataP@Sven-27
(HTML file). I would put the card-container in a main element. Because that is the important part of the page.
The element with the hook class would be better to store it in a h1 element. Because that is the title. and the cta class i would put in a p element.
That way it is more clear what the purpose is of the elements in your structure. It also makes a big difference for screen readers.
(CSS file). In the new css you can structure the css like you do in the html. Like SASS. That way you can structure your css better.
I would give the body always a min-height incase you have pages that need more room then the 100vh. And to center the card it is shorter to use display grid with place-items: center. For this case anyway.
Other then that it looks good.
