Looking to hire developers?
@edburtnieks-private
@PaulyTee
Submitted
I started learning due to the lock down following a course or 2 on udemy.
This is my first challenge on Frontend Mentor so any feedback would be good.
I had to focus quite a lot on learning css grid and making the design responsive to complete the challenge.
I'm still unsure on text scaling and how that works with responsive designs so any insight would be good.
Many thanks,
@edburtnieks-private
Posted
Hey Paul :)
Your solution looks great! After a quick look, I found couple small things.
I noticed that you have headings h3 being smallest and h5 being largest. I don't think in this design there is need for more than 1 or maximum two headings - h3 (h4). Generally, you shouldn't chose headings based on their style (color, size, weight). Now, in design you come that across quite often, but not always. I think in this case it makes more sense to stick just with single h2 element, one for each section inside the card.
From you CSS file.
Just an opinion, but if you put color values inside css variables, I think it would make sense to make white color a variable too. For example line 72, you have #ffffff as color value. I would put it up with the rest of color variables at the top.
I noticed you removed outline for button element. In general you want to avoid that. Or if you do remove outline, then add some styles inside :focus selector for button. That way if you are using keyboard to tab through your page, user will know when their focus is on the button.
For some reason you have two box-shadow properties inside .container selector. line 33 is overwritten with line 38.
You could simplify your layout and reduce CSS file by applying mobile first approach. That is, keep all the card sections display: block; as they are by default because of div elements. They will naturally by default stack on top of each other. And then you can apply grid only inside media query on larger screens, where you need it. Note that you would need to change max-width to min-width inside your media query.
Answering your question about responsive text scaling. You have couple options. You can simple change the font-size property inside your media query. That's probably the easiest, simplest and most predictable way. Almost always I do it this way.
You could also look up some fluid font size scaling with % units. I haven't used fluid scaling much, but it has some uses, depending on how you want your responsive design to work.
I hope I cleared up some things. If you have any questions, feel free to ask here or on Slack.
Nice work, keep it up! :)
@rubengomex
Submitted
I have a tiny issue with the top border of the cards on table port.. I couldn't find the solution for that. If you can explain the possible solution would be nice. thanks
@edburtnieks-private
Posted
Hey Ruben.
The issue occurs because you are applying border to ::before pseudo element, leaving the background color transparent. Adding background-color property with the same color as border fills in that empty gap.
I would remove border and border-color properties completely and just add background-color with specific height. It will give the desired effect without any issues :)
@NatClamp
Submitted
Is my SCSS efficient? How browser-compatible is my code? How could my HTML be more semantic - have I used the HTML5 tags correctly?
@edburtnieks-private
Posted
Hey Natalie :)
Your styling with SCSS in general looks great. There are some inconsistencies with variable names. Neutral colors are PascalCase whereas rest are camelCase. Also, it's not necessary to put tag selector in front of class selector for sections.
As for semantic tags and accessibility. Only thing I noticed are alt attributes for icons. You should have some meaningful text that describe the icon itself. Or if you want to leave as purely decorative and not for screen readers, you could leave alt attribute empty.
Otherwise everything looks good. Nice work :)
@TheDeveloper-Manish96
Submitted
Hello Everyone This is my first project "Please tell me how I can improve” please check the project and give your valuable feedback .
@edburtnieks-private
Posted
Hey Manish, solid solution and looks great!
Having a quick look I noticed some small things that could be improved.
First, I would rethink how you structured your heading element. In general, you want to avoid skipping heading tags. For example you have h4 after h2 in .item1. I think it would make more sense to use only h2 and h3 or even just h2. For Join our community, Monthly Subscription and Why Us leaving rest as p or small elements. You shouldn't choose heading elements based purely on their color and size. Most often yes, but not always.
Another thing, you can try applying styles with "mobile first" approach, I think it would reduce your CSS a bit :) Continuing about responsiveness, you should extend breakpoint, because right now on smaller screen sizes, applying desktop styles, text inside two columns at bottom is really narrow.
Other than that, nice work. Keep it up! :)
@adarshcodes
Submitted
Hey everyone, I worked hard on this challenge. I'm uploading this second time as the first one doesn't look at how I want to make it. Let's see what I got now, If you have any suggestions or recommendation or anything which can be modified, please tell me, I'd love to hear from this amazing community. Any suggestion on how I can fix this HTML issue
@edburtnieks-private
Posted
Hey @Adarsh.
The design is spot on! The crazy animation on page load made me laugh:D
You did not use any SASS or CSS Modules or any other css compilers, right? I see that you css is 1000+ lines of code, so I would probably think about splitting it into several smaller files. I like that you at left some comments on different sections inside css file :)
About your HTML issue, you are missing an element with id #subscribe-btn. In the repo the input element has aria-labelledby="news-heading" though.. But the aria-labelledby looks for the element with id of the same name.
@NDOY3M4N
Submitted
Hi, I just wanted to know how to integrate Zeit Now to my repo. I've tried to import this project through my Zeit Dashboard but it's not working. I was forced to use Github Pages.
@edburtnieks-private
Posted
Hey! Essentially it doesn't really matter how you deploy your solution :) How did you try to import the repo in Now dashboard? Matt just couple days ago added a compete guide for submitting solutions including some instructions to deploy it on Now.
https://medium.com/frontend-mentor/a-complete-guide-to-submitting-solutions-on-frontend-mentor-ac6384162248
@michalgregu
Submitted
Looking back, i would definitely use grid instead of only flexbox, wanted to have some practice with flex though. Thanks for looking!
@edburtnieks-private
Posted
Hey @Michal!
You solution looks good :) I think it's great that you chose to use Flexbox in most parts. I actually would have used Flexbox too :)
Just quickly looking through.
I noticed you have justify-content: flex-end; on header. You don't need it if you have margin-right: auto; to logo :) Or you could remove margin-right from logo and leave justify-content.
Nice work, keep it up :)
@alanhcrdz
Submitted
I'd like to have some specific feedback on spacing, I think I did too many margins and paddings on classes. But I'll be glad to have general feedback also. Many Thanks!
@edburtnieks-private
Posted
Hey @Alan!
Design looks pretty good! For spacing I use browser extension called PerfectPixel. With it you can overlay the original design image on top of your website, change opacity, toggle it on or off. It is pretty great tool specifically for making "pixel perfect" design. As you practice more and more you should be getting better and more consistent with guessing spacing :)
@MeFredj
Submitted
Hi all,
Please be so kind to let me know how I can further improve my coding skills.
Kind regards,
Frederik
@edburtnieks-private
Posted
Hey Frederik! What exactly would you want to improve and focus on?
@inkinkz
Submitted
Feel free to leave any comments!
@edburtnieks-private
Posted
Looks really good and spot on!
Couple small things I noticed.
For card buttons, I would use actually button element:) Wanted to tab around with keyboard and couldn't:( Also, I would add some :hover and :focus styles for toggle at the top for the same reason.
One other thing, between screen width 861px and 1010px the cards are right against the side. Maybe put some padding around:)
Otherwise, nice work:)
@mihneagtt
Submitted
I'd like a feedback regarding the code, as this is my first website and I'd like to know if I wrote a "clean code". I know that my attempt to make it responsive was a fail, but I'll study more and I will fix my mistakes in my next project. Also, any suggestions or opinions are very much welcomed.
@edburtnieks-private
Posted
Hey Mihnea!
The project looks nice, especially for the first website. Good work. Couple of things.
First, as you see in your report there are some issues with img elements missing alt attribute. You should always provide alt attribute:) If the image is purely decorative (in this case there are none) you can leave the alt attribute with empty string like alt="". That's totally fine, but it's good practice to always include it.
Keeping on semantic HTML topic, there should not be two h1 elements on single page. h1 element represents the main title of the page and it only makes sense to have one main title per page. If you need to break the title in several lines, as in this case, you can put <br> between the words you want to break.
One other thing you could do is to change <div class="flex"> to <main> element as it is the main content of your page.
I also can see that you have quite a few unused css classes (e.g. .flex and .container) and unnecessary wrapper elements. Try to get the result with as few elements as possible and add them later if necessary.
As for "clean code". I notice there are some inconsistent tabbing inside HTML file. Inside css file, you have indentation set to 4 spaces instead of 2 as you have inside HTML file. It's good practice to stick to either 2 or 4 spaces and be consistent though project, even in different files. Also inside css file, there are a lot of inconsistencies regarding spaces before : and {. In some places you have space before/after, in some you don't.
For these issues you can look into file linting and formatting:) You won't need linting much for this case, but file formatting using for example Prettier is definitely very useful and keeps your code clean automatically once you set it up;)
One side note, it's good practice to have your css file be the last <link> inside <head> element. In this case it wouldn't matter because you are only getting fonts and icons, but it's good practice, if you have external css files (not yours), they should be before your ones:)
Aaand finally you should have atleast :hover and :focus for the button;)
These are the major things I picked up on. Keep it up!:)
@jordanchude
Submitted
Is Flexbox or CSS Grid best to use here?
My logos, when the page gets smaller, go under the boxes that they're in. Is there a way to make them stay in their containers even as the page gets smaller?
@edburtnieks-private
Posted
Hey Jordan,
Apart from your questions, I have couple of ideas and advices. First, for .intro and .container section elements, you could use header and main elements instead.
For the title, I think it would make more sense in this case to have it just an h1 element as I don't think in design they are supposed to be different headings. You could wrap the bold part in span element and style it.
You are missing the mobile design too, which you can achieve with media query :)
Answering your first question. It depends on how you want to do it:) There is no right or wrong here. Try with Flexbox as well and think how you would solve it. If it takes more time, then stick with what feels faster and easier to write for you. I personally used grid here as well, as it makes it very clear from the first sight you look and you don't have to move markup around to position the cards. But you also need to keep in mind browser support and decide.
One tip, instead of doing grid-template-columns: 1fr 1fr 1fr; you could write it like grid-template-columns: repeat(3, 1fr);. And same for grid-template-rows. Personal opinion, but it looks nicer, more readable and understandable:)
As for the issue with logos going outside the box. I see that you have written a little bit of unnecessary code and not taking advantage of grid doing all the magic.
I would remove from .card element width, height, justify-self properties. Notice, if you leave width: 100; the cards are overlapping each other. That is due to padding on each card. You would need to set box-sizing: border-box; in order to not add padding to overall width of the elements. You will come across a lot of people who just set it for *, *::before, *::after selectors and that's what I do in most cases too. But as you are using grid, the width is not necessary. About images going outside the card. You have a set height for cards - 250px. The container won't adjust it's height to accommodate the content, because you specifically set it to set value. I would avoid setting height property as much as possible, because as you can see, you restricted yourself for responsive design. If you need, you can set min-height instead, so it will always be 250px, but if the content won't fit, the container height can extend beyond 250px.
Also, notice how currently you have set the specific margin-top: 50px; for icons in the cards. Meaning that the icon won't always be at the bottom right corner if height of the card will change. Try to think of solution to have the icon always be at the bottom right corner no matter the rest of cards content height.
@developerbottle
Submitted
I've finished this challenge. If you have any improvement suggestions or interesting alternative ways of doing smth, I would be glad to hear them! Code reviews or any other feedback are appreciated.
@edburtnieks-private
Posted
Hey Shoaziz,
Your solution looks really good and clean! Just couple really small points and details I could point out. I like that you made it different for tablet size.
In design, the colored top borders doesn't have that slight curve at the bottom edge when the border meets the edge of card, as you have it in your solution. It's more like a straight line. You could do it with ::before pseudo element instead of top border. But it would a little bit more code, and it looks perfectly good with the current solution. Just a very small detail.
Second, try to see what happens when you extend text content in cards for more than two lines. I know, in design in each card there are exactly 2 lines of text, but let's imagine the content would change in future. Currently the cards won't be the same height on tablet and desktop sizes. Keep in mind that icons could have different spacing above them, not set value (4rem on desktop in your case)
Just something to play around, it shouldn't be difficult to implement.
Otherwise, nice work:)
