Jennifer Souza
@zoedarkweatherAll comments
- @Gioant@zoedarkweather
Hi, my understanding is that pixel-perfect is kind of impossible and isn't as important as faithfully implementing the spirit of the design. It can even actually cause issues (not least of which is making you frustrated lol). Here is a good article on this idea https://www.joshwcomeau.com/css/pixel-perfection/.
Regarding font size - my understanding is that the body text font size refers to paragraph font size. 16px is the default p font size though so if it says 16px you don't need to specify that. Also, even though the style guides will have a font size in px, it's best to translate that into rem in your css to make sure it's accessible. Hope this helps.
Marked as helpful - @HakanaiKC@zoedarkweather
Hi, to remove the border on your button, instead of outline:none, set border: none. Outline is different than a border - it refers to the outline that appears when an element has focus. And you should, as a general rule, not remove the focus outline because it causes accessibility issues. Hope this helps.
- @KalabSisaySE@zoedarkweather
Hi, looks pretty good overall. I agree with the above comment that mobile-first is the way! I'd also point out you can use the picture element in your html to switch out the image for the correct screen size. Here's the mdn reference page on that if it might help: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture. Best!
Marked as helpful - @Mr-Funderburk@zoedarkweather
Yeah I also thought the background was a bit aggravating on this one. As far as I could tell from the design, the images are actually in different positions for the different screen sizes. So I don't think it's a matter of finding the perfect spot where they will just shift on their own. What I did, was change the background position in a media query. Hope that helps. :-)
Marked as helpful - @CesjhoanFeliu@zoedarkweather
You're off to a good start doing mobile-first. I'd get out of the habit of pixels as soon as you can - I know it maybe feels like there's less control that way, but it really is key if you want to build something responsive. It's also more accessible. For resources to help with responsive layouts I recommend Kevin Powell's free course Conquering Responsive Layouts. https://courses.kevinpowell.co/view/courses/conquering-responsive-layouts. Helped me understand flex box a bit better and also when to use rem vs em. Also really anything on Kevin Powell's youtube channel also - just search for responsive or grid or flexbox... Hope this helps. :-)
- @moncadad@zoedarkweather
The background was kind of tricky. It's two images, set both to no-repeat. There's one that is offset from top left and one that is offset from bottom right. You can take the dimensions of the images to compute the offsets. The MDN docs on background https://developer.mozilla.org/en-US/docs/Web/CSS/background I find helpful. Also here is a link to my solution if you want to look at my css for the bg. https://github.com/zoedarkweather/profile-card-component. Hope this helps. :-)
Marked as helpful - @Genii-X@zoedarkweather
- You could select based on which child it is of the parent (nth child) or something but that is seriously over-complicating when you're already selecting the element to style it with a background color, etc and it's like one more line. I think the way you are doing it is probably the best way to do it.
- If you start with mobile and do media query for desktop everything is usually simpler. In this case, you could maybe just let your flexbox wrap. You'd probably still need a media query though to change which corners have border radius.
- I'm sorry I'm not sure what you mean here exactly on this one? To put your elements in a horizontal row, you can use flexbox or grid, and flexbox is often the solution for one-dimension like this. So I think you're doing it right using flexbox here! Hope this is helpful. :-)
Marked as helpful - @omarmohames@zoedarkweather
It is stretching a bit at the 450-800px range - I'd either set the media query for a smaller width or add another media query to handle that break point. Also, I'd work on trying to be a bit more descriptive with your class names as it makes your code a bit difficult to follow. Classes named one, two, or a, b, c, d maybe made sense in the moment, but it has no meaning for someone else trying to read your code or maybe even you if you come back to it a few weeks from now. Hope this is helpful.
- @lukasdevit@zoedarkweather
Hi, I'd say "maybe" to too complicated JS? Probably putting it in one file would improve readability and maintainability. Also, you have duplicate code - page.js and newsletterFormSignIn appear to be the same file with different names. One thing - you can hide the success message using absolute positioning and moving it offscreen to avoid writing a bunch of html with your JS and that would simplify things a lot. Another thing I'd point out is that you can simplify your validation - you have your input set to type email and you made it required, so you don't need to test against any regex or anything. You can use the built-in Constraint Validation API. So you'd test input.validity.valid which returns a boolean indicating if it is violating any constraints - in this case missing or not an email. You could test further as validity returns a validity state (eg typeMismatch or valueMissing), but in this case you don't need to get more detailed since you're setting the same error message no matter if empty or not an email. Hope this is helpful.
Marked as helpful - @AhmedAbdalazeem49@zoedarkweather
Hi! I see what you mean about the box with the price, but I think it's really that the whole flex container is collapsing on mobile. In your media query, I'd either do something to set a min-width on your flex container, or the better solution is probably to set your flex items to flex:0 so they don't shrink. Hope this is helpful!
Marked as helpful - @Alfrey-Chan@zoedarkweather
Hi! You can use the HTML picture element when you have images you need to swap depending on screen size. No need for media queries if you use picture. Also as far as I know max-width 50% is fine to do when you need to? However, I would try using the features of grid or flex box for something like that. With grid, for example, you can set your columns 1fr 1fr and they'll be 50%. Or with flex box you could do flex=1. Hope this helps.