P
Marley Reyna• 510
@MarleyReyna
Submitted
Let me know what I can improve. The sass felt a bit messy and there may be more to get better at.
Jimmy Sweeney
@sweenejp
All comments
- Jimmy Sweeney• 350
@sweenejp
Posted
Looks good Marley. One critique I have - could you put the infoUnavailable method within your loadUser method? That way you wouldn't have to put infoUnavailable in your JSX. That way you would have all of the data ready to be accessed when you render the component. Probably not a big deal, but I think it would make it a little cleaner.
Marked as helpful
0 - Gregg Christofferson• 295
@gchristofferson
Submitted
Hey everyone! 🙋♂️
This was my first 'Junior' level challenge I have tried and I have to say it was a little difficult for me, mostly due to the multiple images and shapes a that are placed off-screen in different positions at different breakpoints. I'm wondering if I used the overflow property correctly to get this to work? Also, this is the first time I've used the
<picture>element. Did I use it correctly and is this the best option when I want load different image sizes and different screen sizes? Lastly, I feel like I wrote too much code, both CSS and HTML. Is there any way I could have made my code more 'DRY'? Did I miss anything else?Thank you for your feedback! 👍
Jimmy Sweeney• 350@sweenejp
Posted
Hi Gregg,
Your solution looks pretty close to the design so good work.
One thing I notice right away is that you could have all your media queries at the end - that way you would just have two media query lines and then put all of the classes that need to update at different sizes within those media queries (instead of having a media query on all of your classes). That might just be a matter of preference though.
Another solution to loading different image sizes is to create a div with a
background-image: url(file-path.jpg)and then update the file path with media queries..info__img-wrapper--brand { transform: translateY(13px); }- don't know that this line is necessary.
I could be wrong but it doesn't seem that overflow is doing anything. Looks like you achieved the effect of getting images to be off screen with
flex-wrap: no-wrapwith one of the images and a negative margin with the image. Maybe there was a way to be more consistent here and use no-wrap to achieve the effect for both images?Keep up the good work!
2 @ichiklaus
Submitted
Hello! This is my solution for this simple challenge. I try to make it responsive for most common resolutions, following a guide from Bartholomej (acknowledgment is in readme). Tell me if I'm doing the right approach + any feedback is welcome.
Jimmy Sweeney• 350@sweenejp
Posted
Hi Nicolas,
Looks like you need to adjust your media queries. Typically you don't want any side scrolling. The layout breaks at around 1300px. Personally I think it's better to set media queries based on when your layout breaks - not based on typical sizes of screens.
You probably just need one media query here if you take a mobile first approach. Create the mobile layout, then set a
min-widthmedia query for when you want to transition to the desktop layout (somewhere around 1300px probably). Then make the necessary layout adjustments under that media query (flex-direction: row;, etc, etc).Marked as helpful
0@DanielJvV
Submitted
Made this web page for 375px and 1440px screen sizes. Just wanted to know if my code is acceptable out there in the real world haha. What I could improve to make it something that a paying customer would be happy with. Any advice basically. Thank you in advance.
Jimmy Sweeney• 350@sweenejp
Posted
Looks good to me. One thing I noticed is that the social media icons don't change color on hover.
You could also try keeping the text within column 2 from condensing when you resize the window.
Marked as helpful
0- Riyana Gueco• 495
@rngueco
Submitted
Hey!
I wasn't too sure what to put in the JavaScript validation side of this project, but I removed the
requiredattribute for the emailinputelement to prevent the:invalidstyles from firing right at the start and did that check on the JavaScript side instead.Aside from that, a simple pattern matching using the regex seen from MDN's docs.
I consider myself a JavaScript newbie, so please advise or give feedback. 🙏
Thanks!
Update: I have updated my solution based on feedback. The email field is now
required, but the error message will not fire unless the user has focused and then removed focus on the field. Additionally, validity is now being checked before form submission, returning the appropriate error message based on whether the email input is empty or invalid. In case of user bypassing therequiredfield and submitting an empty input anyway, an extra validation step is still checked on form submit.Jimmy Sweeney• 350@sweenejp
Posted
Nice design! You really matched the solution well.
I took a little different approach to the validation. I left on the
requiredattribute because I think this keeps the error message from showing up until the user hits the submit button. Then I added an eventlistener with an event type of "invalid". Got help with this from this blog post here: https://daverupert.com/2017/11/happier-html5-forms/You can check out my solution here if you'd like: https://github.com/sweenejp/base-apparel-coming-soon-page
Marked as helpful
0 - Afolabi Oseni• 105
@GoldenAceTech
Submitted
Hello there! This is actually my first project on Frontend Mentor. I just started my Web Development journey so I will appreciate your feedbacks. Let me know what you think and criticize if you have to. Thank you for taking your time to check, have a good day!
Jimmy Sweeney• 350@sweenejp
Posted
One small detail is that I think the card has a really subtle shadow effect. Other than that, looks great!
I like how you used grid to set up the layout of the card. Works well.
2
