Looking to hire developers?
@elaineleung
@dboca93
Submitted
Hey there ! Would love some feedback on this task. Especially as to whether my use of sematic html was appropriate. If there is anything wrong with my code, I'd be more than happy to take on your suggestions. Thanks so much !
@elaineleung
Posted
Hey Dilhan, excellent job here, and it makes me want to go back and clean up some of my old code for this challenge!
The semantic HTML looks pretty clean to me :) In terms of suggestions, one I could think of would probably be just to add a header tag even though it can be optional, but it would be nice to have since you got a main tag with an h1 in there, and I might put that h1 under header instead. (By the way, what you did with the h1 was also something I did for a lot of these component challenges... I think that's a good idea too!)
One other thing I saw (which was also something I did in my own solution) was putting the stats in p tags. If I could do that differently now, I would have the stats as an ul list instead, as that might be easier to pick out for users with a screenreader.
Once again, great work!!
Marked as helpful
@dhan5a
Submitted
I don't think I got the font-sizes correct, compared to the design.
Also, I tried using root values for the colours, but in the end I had to use the hsl values as the values were not recognised.
Would love your feedback!
@elaineleung
Posted
Hi Dhanya, first off, I think you did a great job putting this together! About the custom variables not working, it looks like there's a trailing curly brace in line 3 of your CSS file, and I think if you try to remove that, you'd be able to use the variables (I tried it in the browser just now using your code).
Some suggestions I have here: I'd remove the width: 50% from the body selector, and if you're hoping to put some space on the sides to prevent the component from touching the browser sides, instead of having max-width: 300px on .card, you can try width: min(90%, 300px). This declaration tells the browser to keep the width at 90% until it gets to 300px, at which point the width would be maintained at 300px as the max width.
Lastly, about using fixed pixel values in font sizes: It's generally good practice to use relative units (such as rem), so that's something I encourage you to try as well (by default, 1 rem is 16px). One of the main reasons is because relative units allow users to have their own preferred browser/system settings and won't be locked into using fixed sizes in the code.
(Oh, and be sure to fix add alt tags in your img elements, otherwise these would show up in your report for sure)
Hope this helps you out!
Marked as helpful
@Dytoma
Submitted
This a solution to the frontend challenge on interactive rating component. I used Javascript on this challenge but I don't know if this is best practice so if you have time check out the repository and I would appreciate having any feedback, tip or remarks. Thanks.
@elaineleung
Posted
Hi Dytoma, this looks like a great start, and I think your code works fine. I do suggest giving your classes and variables more meaning names, where instead of
const rating = document.querySelectorAll(".nbr");
... you can try something like this:
const ratingBtns = document.querySelectorAll(".rating");
// note that Btns stand for buttons since there is more than 1 button here!
For the for loop, I suggest using classList instead of className so that you can easily add or remove a class instead of changing the className attribute directly. I also don't think selected needs querySelectorAll since there's only one button being selected. You can try this:
for(let i = 0; i < rating.length; i++) {
rating[i].addEventListener("click", function() {
let selected = document.querySelector(".selected");
selected.classList.remove("selected")
this.classList.add("selected");
});
}
Lastly, instead of having a default score of 1, I would just leave it empty and let the user choose. Sometimes users might accidentally click on the submit before they select the score, and you wouldn't want them to accidentally send a score of 1! Hope this helps you out a bit 🙂
@trunglam7
Submitted
Any ways to improve my approach to this challenge?
@elaineleung
Posted
Hi Trung, I think this looks quite well put together! If there's one thing I'd change, I would use a class to change the style of the selected button instead of using the :focus pseudo class, seeing that focus is meant to used for showing which element the user had last interacted with. If you accidentally click on something else like the background or the text, the selected button would go back to its unselected color because it has lost the focus, and that would make it seem like no button got selected, even though in the background JS is still keeping track which button got selected. I would just create a new class called selected, style the selected button with that class, and then use JS to add/remove the class as needed. Hope this helps you out a bit!
Marked as helpful
@Owura-56
Submitted
The comment arrow was a little problem for me and this is my first time using media queries and it really gave me a headache views and suggestions are welcome
@elaineleung
Posted
Hi Agyemang, good job putting this together! For the triangle with the ::after pseudo element, instead of having just border: solid transparent, try specifying the sides, like this:
.grid-item-3::after {
top: 98%;
right: 0; // remove left
border-left: solid transparent;
border-top: solid white;
// rest of your code
}
Also, change the border-radius in .grid-item-3 to border-radius: 8px 8px 0 8px;, which should give you that sharp corner instead of a rounded one.
With the media query, I'd choose a much higher breakpoint since right now when the layout changes, the sides are cut off. Also, there appears to be some overflow going on, and I think it may have to do with the large margins you're using (such as in .grid-item-3) to do positioning. I would actually suggest not having .grid-item-3 as a grid element but to have that as a child of .grid-item-2 instead, and I'd use position: absolute instead of using things like margin-left: 21rem, which is what I think is causing things to be pushed off to the right side of the screen.
Hope this helps you out!
@jodyvanhoose
Submitted
Just looking for some general feedback. Thanks!
@elaineleung
Posted
Hi Jody, first off, great job in getting your solution to look very close to the design! 🙂 I think you did many things well; for instance, I like how readable and well-annotated your code is. There are just a few things I noticed in the area of responsive design that you can check out:
Breakpoint: The breakpoint is quite large, and you'll notice that the photo in the mobile view gets pixelated past its width of 686px, plus having the mobile view at such a wide width is not very optimal for this card component, as the photo would be gigantic while the lines of text and CTA button gets stretched. What I suggest is to use a smaller breakpoint: to find ideal point, simply see how wide the desktop view is meant to be and use that as a starting point. From what I recall, the design's desktop width should be about 600px.
Image distortion: The image at the desktop view is getting distorted, meaning that the original aspect ratio is being changed. To make sure the image doesn't get stretched or squished, add a object-fit: cover to the image class.
Component width: Aside from changing the breakpoint, one other thing you can try is to ensure the mobile view card width doesn't get too wide. To do that, instead of just having width: 93%, try width: min(93%, 400px); which tells the browser to keep the width at 93% if the width is smaller than 400px (you can use any value here you wish). Likewise, for the desktop view, try width: min(93%, 600px); instead of width: 35%. Once you do that, you may find that the width property for your desktop image doesn't have to be 25%; in fact, it can be 100% or 50% and still be the same width because you have flex: 1 as a declaration, and you also have a width: 50% for your description container.
Hope this can help you out!
Marked as helpful
@ahmad-cod
Submitted
Why doesthe register btn and logo, change position if features or company dropdown is clicked?
@elaineleung
Posted
Hey Ahmad, well done in building together this solution! To answer your question about why things shift when you click on the dropdown, it's because the dropdown has a position: relative. That means that when it appears, its height is part of the header height, and so everything shifts according to that height change.
To fix this in the desktop view, simply change the position property in .dropdown to position: absolute instead and add position: relative to your .primary-navigation li selector, so that the browser knows which parent to anchor .dropdown. to in the positioning. You may also have to change the left and top properties as you see fit. I'd also suggest adding a width: max-content; to make sure the content takes the whole width and not need to be pushed to the next line. For the mobile view, be sure to keep that position: relative in the .dropdown when clicked, like how you had it.
Hope this helps you out!
Marked as helpful
@lily-oliver
Submitted
Hi, friends!
I'm very new to HTML and CSS but decided to try out Frontend Mentor. I've been following a Udemy web development course, and this is the first time I've really strayed away from it. I definitely felt the gaps in my knowledge. Until I did this project, I didn't really understand nesting in CSS. I think this has given me a better idea. Also, centering items within the body of the page is particularly hard for me. It took a lot of Googling to figure it out. Any pointers on how to remember what to do?
I'm super eager to learn more, so please let me know if you have any feedback at all!
P.S., I wasn't sure if that was a drop shadow or compression artifacts around the card... so I added a little shadow.
@elaineleung
Posted
Hi Lily, first off, welcome to Frontend Mentor, and congrats on completing your very first challenge! It's a huge step when you go from tutorials to actually writing your own code, and I must say that for your first challenge, you've done an excellent job!
About your question on centering items, it kind of depends on how you want things to look. Some people want their attribution as a footer pushed all the way to the end of the page, and some people might want it close to their component, which might be somewhere in the center. In your solution, you had your attribution within the component. Each of these cases could have slight variations in how the CSS is written, but regardless what the case is, the key thing that most people tend to forget is the height, which is very crucial because the browser needs to know how much space there is so that it knows where to place the component with even spacing around it. Even when people do remember to add the height, the mistake that most of them make is that they they use height: 100% or height: 100vh instead of min-height: 100vh.
In any case, for the most basic kind of centering where you want everything in the center, here's what you can do:
// centering everything in the middle using flexbox:
body {
min-height: 100vh;
display: flex;
flex-direction: column; // this is needed if you have more than one child in the body selector
align-items: center;
justify-content: center;
}
// centering everything in the middle using grid:
body {
min-height: 100vh;
display: grid;
place-content: center;
}
I encourage you to play around with both flexbox and grid so that you understand how they work because I also struggled a lot in the beginning with how to use them for centering things, especially when there are other components involved, like a footer and header.
Anyway, aside from comments on centering, one suggestion I have for your solution is to add a 1rem margin around it so that for smaller browser widths, the sides of the component won't be touching the browser. I really think you did a great job on the whole, and the real challenge will come when you need to work with responsive design (as in building a mobile view and desktop view), but no worries, I think you'll be learning lots when you put your skills to the test!
Marked as helpful
@ArthurKingDev
Submitted
Hi everyone! I finished the product preview card component challenge. kindly take a look and give me feedback if there is something I need to do better. Thanks.
@elaineleung
Posted
Hi Kingsley, good job putting this together! 😊 I got a few suggestions for you here:
Instead of using background-image for handling the switching of the desktop and mobile images, try to use the picture element instead since this is a product image and is thus central to the card. The background-image property is suitable for images that really are background images, usually with some text on top of the image. For the picture element, you can try something like this:
<picture>
<source media="(max-width: 375px)" srcset="image-mobile.png" />
<img src="image-desktop.png" alt="Glass perfume bottle placed against a leafy background" />
</picture>
I suggest a higher breakpoint than 375px. The 1440px desktop view and 375px mobile views are references for you to see how the component should look like that these two views, but they aren't meant to be the max width or breakpoint. As the developer, you'll have to have the judgment to see which point to use so that when the layout changes, the component can still have optimal views.
Right now the component isn't so responsive due to fixed widths in certain areas, and as a result, there is content that's being covered by the browser, causing overflow to happen. A big part of this could be the fact that you're using background images right now, but you can also try what I mentioned about changing the breakpoint as well.
To make the image width even with the text in the desktop layout, try adding flex: 1 0 50%; on the product-img.
Hope some of this can help you out, and really great work on the whole, so keep going!
Marked as helpful
@debriks
Submitted
Hi Guys! Received some very helpful feedback and refactored my first solution. I improved the HTML markup, changed some values that were still in px to relative units, removed the 'alt' attribute in the images as it was only decorative and changed the 'cursive' font property to 'sans-serif' to avoid getting 'Comic Sans'. Let me know if I missed something. Big thanks again to Lucas and Deniel!
@elaineleung
Posted
Great work Deborah! This looks really good, and I think the responsiveness is quite well done. I just got two suggestions:
At around the 660px breakpoint when the layout changes, the sides of the component are touching the sides of the browser, so I'd probably just add some spacing to keep that from happening (maybe margin: 1rem would be good enough!)
Your report is giving you some issues about missing alt tags, and actually even for decorative images, you still need the alt tag in the img, but you can keep it empty, like this: <img scr="image.png" alt="" >
Once again, well done here, and I'm glad you got improve upon your old solution!
Marked as helpful
@ajay117
Submitted
Guys, I will be very grateful to have any feedback on this challenge...
@elaineleung
Posted
Hi Ajay, well done in building this component! I think there are many things you did well, including making the component responsive, having optimal views, making everything functional with the JS, and having descriptive class names. I found your code to be clean and easy to read, so great job here!
I think the only suggestion I have is that instead of using li for the buttons, try to use either a button or radio inputs, as this would have with accessibility since these are control elements and can be used with the tab key, whereas the li element could not be used with a tab key. Even though you can add make them into focusable control elements, there shouldn't be a need to do that when there are more suitable elements, such as button.
Hope this helps and once again, great job!
Marked as helpful
@Frozennn
Submitted
Here is my solution to this challenge.
Any feedback or suggestion is more than appreciated :)
@elaineleung
Posted
Hi Marko, I think you did well on the whole! The only three suggestions I have are as follows:
The ratings and the testimonials are stretching out quite a bit before the browser width reaches the breakpoint. You can either try a smaller breakpoint, or better yet, use a max-width for the cards and ratings. I see that you have a max-width on the ratings but it's in a percentage, which means it will still continue to stretch. What I would suggest is to use a relative unit here, like rem. For starters, try max-width: 25rem and then adjust accordingly.
At desktop view, the site main heading has quite a large line height. I tried changing it via the inspector but I was not able to for some reason. In any case, it would be great if that spacing is a bit smaller!
Instead of using the picture element for the SVGs, try having them as background-image instead because they are merely functioning as decor elements, and they aren't really there to enhance the site content in any meaningful way.
Hope this could help you out!
Marked as helpful
@Abdulazeem-code
Submitted
I tried my best thou. i open for correction
@elaineleung
Posted
HI Shobiye, it looks like your live site isn't working; the link doesn't appear to be the right one, and when I went to your repo, I don't think the GH pages is properly set up yet. If you need some help with that, check out Matt's post here on how to submit solutions: https://medium.com/frontend-mentor/a-complete-guide-to-submitting-solutions-on-frontend-mentor-ac6384162248
You can also try Vercel, as that's one of the easiest ways to deploy your code.
If you need more help, just let me know by replying to this message!
@krutagna10
Submitted
@elaineleung
Posted
Hi Krutagna, first off, great job in making this solution responsive, and I also think you did many things well here, including how you used grid and also relative units instead of pixels.
About your question on good CSS practices, here's a short list on what I consider to be good practices in general:
Hope this helps you out a bit, and great work in general!
Marked as helpful
@RahulPaul12
Submitted
learning many things
@elaineleung
Posted
Hi Rahul, your links for the live site and repo don't seem to be working, unfortunately! I managed to find your repo, but I can't access your site as I can't find the Netlify link at all. Hopefully you can fix those first, and then we can see how to give you some feedback!
@David-Henery4
Submitted
Although I managed to implement the flip animation, I felt I could of done it a little bit better. So I was wondering how everyone else implemented this? Did you do it by plain CSS or a library? Also how do you think I could improve the animation I have?
Any sort of feedback would be much appreciated, Thank you!
@elaineleung
Posted
Hi David, well done in building this project in React, which seems to work quite well to me! The only comment I have is that, when the numbers switch to the next time unit (e.g., counting down from the "01" second mark), I think it should be showing zero. For instance, after "01" in the seconds column, I would expect to see "00", but instead I see "60". In short, I suggest that the range should be from 0 to 59, not 1 to 60.
As for other solutions, I really like what Curtis did his solution, and even though it's a plain JS solution and not in React, I think it's still a good reference, so check it out here: https://www.frontendmentor.io/solutions/responsive-countdown-timer-made-using-vanilla-js-sass-3wnMC395Zn
Marked as helpful
@HDanielO
Submitted
what else could I have used for the solve button..I used the eval() method
@elaineleung
Posted
Hi Daniel, good attempt in building this project, and I just wanted to answer your question about eval():
I also used eval() at first, but after reading up more about it on MDN and reading Stack Overflow posts that explain why it shouldn't be used, I ended up using a switch statement for the four operators. Here's the link to the MDN article on eval(), and I highly recommend that you read it and consider whether to use eval(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval
Anyway, I tried out your calculator, and I really only got three main comments:
I think you need to write a bit more logic in your function for the computation, and I also think you need to have a look at how real-life calculators work (whether they are single-step or expression evaluators). I know there aren't too many specific instructions for this challenge, but I think it should be assumed that this calculator is meant to function in a way that real-life calculators function. What this means is that, let's say I enter "3", "+", and then "5"; I get the answer "8", which is the desirable and correct answer. Then say I wanted to input another number for the next calculation, and that number is "2". What I see on the screen is not "2" but "82" because the display is not cleared and the calculator simply appends the new digit to the previous answer.
Good job in getting the themes to work! The only thing is that, how the JS is written is rather clunky and hard to read, as it's a bit too repetitive right now with all the adding and removing of classes. I also don't think this is the most ideal way to write the theme function. I would suggest that you look into how else the themes can be wired up and do try some refactoring and use of forEach or a for loop.
You have quite a number of issues in your HTML report! Some of involve very fundamental principles being broken, such as id being used more than once. I think you'll need to be careful here and try to remember the basics as you're writing your HTML.
In reviewing your work here, I suggest that you try a few more Junior level projects first that involve writing JS before you tackle more intermediate projects, which I think are a bit beyond your level right now. Otherwise you may find those projects quite challenging! Good luck, and keep going 😊
Marked as helpful
@JoelLH
Submitted
Hey everyone, this is my solution for the advice generator. I'm not sure about my JavaScript code, the quote takes a long time to load and the btn doesnt work right away, not sure if thats something related to the api. If u have any feedback pls let me know, thank u!
@elaineleung
Posted
Hi Joel, about your question on the API, if you happen to be using Firefox like I am, then there's a caching setting in the browser that causes the next advice to not load right away. To fix that, you just need to add an object in the fetch method, like this:
fetch('https://api.adviceslip.com/advice', { cache: 'no-cache' })
One other suggestion here: You can try to change the width to max-width in your quote-card with a bit of margin around (e.g., margin: 1rem), and once you do that, you can try making your breakpoint smaller (e.g., 500px) since the card should be more responsive now.
Marked as helpful
@Senkuu-Midoriya
Submitted
I had a bit of trouble formatting my code and refining it, and I am sure there are simpler ways to do somethings. I learnt a lot about using positioning, and it was a good way to practice flexbox. I would love to hear if there are better ways to do the things I did or simpler cleaner one. Any advice would be appreciated
Thanks to frontend mentor for providing us with such wonderful projects to practice with for free. :)
aDev
@elaineleung
Posted
Hi aDev, great work on this very first challenge! I tried to look at your repo, but firstly the link doesn't seem to be working, and when I did find your repo on your GitHub profile, I couldn't really find the component files. Are they in the .gitignore? I'm really not sure what's happening, but in any case it would help if I could see how your wrote your code. Anyway, I think using Vercel for deployment is good for these projects, and it looks like that's what you're currently using. Are you experiencing some issues with it?
As for feedback on your work, I think the only comments I can make are these ones:
visually-hidden class, otherwise you'll have issues in your report when the button has no text.I wish I can see what's going on the CSS and Next.js but I just can't find the files sadly. Anyway, this is good enough for now, and once again, well done in building this solution!
Marked as helpful
@mays21
Submitted
This is the first time I have tried.
I was not sure which unit of measure to use for styling. I thought "px" would be easiest, but it didn't seem responsive enough.
I could not automatically center the box at the top and bottom. I set margin-top but it might be unbalanced on some devices. I would like to know a better way.
@elaineleung
Posted
Hi May, welcome to the Frontend Mentor community, and great job in completing your very first challenge! 😊
I think @JordanKisiel gave you some excellent advice, especially the link to Kevin Powell's explanation on which units to do, so do be sure to have a look at that when you can. There are times when you may want to use pixels, but for the majority of the time I think relative units are the best choice.
As for feedback on your work, I really think you did many things well here for your first challenge, and the only two suggestions I have are (1) change the first div to main to take care of the landmark issue in your report, and (2) add a margin: 1rem to your .card so that there's some spacing around the card when the browser size gets smaller. I tried adding the margin via the inspector, and then I see that you got some important tags on the margin property. I would say try not to use important tags if you can help it, and in fact, whenever possible, try to avoid using them. If you find yourself needing to use them, then actually what you need to do is figure out which property or rule is cause the declaration to not work as expected.
Hope some of this can help you out, and keep going, you're doing great!
Marked as helpful
@Eltunas170
Submitted
1.- Do you think it is necessary to be practicing tools like Tailwind/Botstrap or continue using only css for the following challenges?
2.- What html tags would you say are necessary for good SEO practices?
3.- Is the MediaQuery better to put in its own css page?
Thank you for all the feedback and comments you can give me. :D
@elaineleung
Posted
Hi Alberto, welcome to Frontend Mentor, and great job in completing this very first challenge!
About the questions you asked:
I think frameworks can be useful, and there's a reason why they're created. As @Souicia said, I do think it's necessary to be fairly proficient in CSS first before using a framework, otherwise you'd have a hard time troubleshooting when things don't go the way you expected. Once you feel more comfortable and confident in building layouts, you can go and try a framework to see how it can help you.
For SEO, I'd say that the important tags are probably title, meta, headings tags (especially h1), and also probably alt tags. I've seen some articles online about this, and so do have a look around if this is something that you'd be interested to know more about.
I prefer having the media queries with the rest of the stylesheet if I'm already having everything there. There will be times where I might have them on a separate sheet, but generally I like having the media queries easily comparable with the main code.
And now for some feedback on your work!
min-width: 1440px is much too big. Given that the card's width is about 600px, I'd probably set the layout breakpoint to be around the 600 to 700 range. I think you can try something like 650px to start!picture element instead of using background image, which is really more for images used as a background. In this case, the image is really central to the content, and so you'd want the alt tags available for the image description (which is something you can't do if it's just a background image).flex: 1 0 50% on .productCard--img.That's all for quick feedback; I really think you did a lot things well here, and this is a great start, so keep going!
Marked as helpful
@julemarts29
Submitted
Hi, I am not good with front-end and tried to learn more of it. I had some issues while coding. I have a hard time differentiating Flexbox an Grid so in my code I only used flexbox. Can I ask you guys some tips on how can I improve and what kind of display should I use for different projects?
@elaineleung
Posted
Hi Justin, congrats on completing your first challenge, and also, welcome to Frontend Mentor! 😊
About your question on grid and flexbox, I think whichever one you use is fine as long as it helps you get the job done at your current ability. Some people differentiate the two by saying flexbox is for one-dimensional layouts (either vertical or horizontal) and grid is for two-dimensional (both vertical and horizontal); there will be times when using one will be easier than the other, and also there will be instances where only using one will work. Ultimately it's all about how you use the tool. For me, I use flexbox probably 90% of the time out of habit, and I use grid for certain things that I feel are easier or requires less code than flexbox.
About your solution, great job here! I think you did many things well, and if I need to give some feedback, probably there's just a few things I'd do differently:
flex-container I would use min-height: 100vh instead of just heightcard, I would use max-width instead of just width so that the card can be responsive. I'd also add margin: 1rem so that there's some spacing when the browser screen is smallcard-body, since this is the container that has the image text, I would put padding: 1rem to make sure there's a uniform spacing around the elements. In this way, I won't need to add any individual margin or padding to other parts. I'd remove the margin-bottom here also, and if you do need some more spacing at the bottom, then I'd probably use padding-bottom.img, I'd change the border-radius to be the same as the card's, and I'd also change the width to 100% instead of a fixed width. What this means is that the image will take whatever width that the container has (minus the padding and margins), and there won't be a need to set its own width.Hope some of this can help you out, and once again, well done!
Marked as helpful
@AnthonyQY
Submitted
Is there a better way to add a purple hue to the image than applying an absolute positioned div? The responsiveness start to overflow vertically when the width of the page is less than 375px, is this an issue?
@elaineleung
Posted
Hi Anthony, I'll try to answer your question on the overflow! I'm actually not too sure what you mean; are you referring to what happens when your card content becomes larger than the screen height, and then you need to scroll down but you see this white space? If so, then that's due to height being used instead of min-height. All you need is to change that to min-height: 100vh and should get rid of the white space.
About whether the purple hue absolute positioning is necessary, maybe @correlucas can shed some light on this matter!
Marked as helpful
@HassanMuhammadd
Submitted
@elaineleung
Posted
Hi Hassan, good job in building this, first of all! I tested the tip calculator, and I think you can try to work on the functionality a bit as right now, when input the numbers and try to change the tip, the amounts don't get recalculated. I see that you're using onblur in your JS, which I don't really recommend as it mainly is used to remove the focus from the element. I would just write a function that would calculate all the totals, and then call that function whenever a button is clicked or an input value is changed. You can check out my tip calculator if you need some ideas on how to do that: https://www.frontendmentor.io/solutions/responsive-tip-calculator-app-with-plain-js-Nj1Gtzub_V
Also, you got some issues to fix in your report, and it seems like adding a main tag should fix most of these issues, so do try that out! You can simply just change one of the topmost div to main, and that should do.
