Suraj Jha• 35
@Suraj9505
Submitted
I am a beginner so please tell me if i can make it with simple method and reduce the code
rikvandersar
@RikvanderSar
All comments
- rikvandersar• 145
@RikvanderSar
Posted
Hey,
Great job on this challenge!
A couple of things I've noticed looking at your code.
- First of there are a couple of accessibility issues reported, you should get that fixed.
- Check your html code first and try to replace the divs for semantic html
- In your CSS your using em units for almost everything. I think em units are particulary usefull for padding and margin in a specific section where you want the section layout to adjust according to the fontsizes in that area. For the height and width of your card or border radius (for example) it might be more logical to use px. Your could use max-width and max-height if you want to keep responsiveness.
Marked as helpful
0 - Carl Wicker• 1,055
@carlwicker
Submitted
Any feedback welcome.
1: I'm not sure why the validation doesn't like "all: unset".
2: Couldn't find a good solution for coloring the SVG social media icons on hover.
3: The generate screenshot function in FrontEndMentor doesn't render the patterns on my backgrounds although they work fine.
rikvandersar• 145@RikvanderSar
Posted
Hi Carl,
Nice job on this challenge.
I don't know anything about React. But I've noticed two things that might be worthwhile to look at.
- The hero image doesn't grow larger as screensize gets bigger. This might not be mentioned in the challenge, but if I go a little wider then mobile width the hero and the footer don't grow / change with it. This might be a nice thing to fix with relative units and an extra breakpoint.
- The other thing is when your mobile menu is opened and then somebody enlarges the screen, you're page doesn't scroll down anymore. This problably has something to do with overflow restrictions and swithing that of in a breakpoint.
2 - GenesisX3• 20
@GenesisX3
Submitted
Struggled a lot doing this one, trying to improve, feedback is welcome.
rikvandersar• 145@RikvanderSar
Posted
Hi,
Just wanted to give you some quick feedback. Great job on this challenge! Just a couple of things you might want to take a look at:
- The div that you called 'container' might be more appropriate to call 'card'.
- You don't really need flexbox on you container div. It is now used to center the content of your card in the middle. But you could also try to fix the challenge without flexbox on the container. If you do that, don't forget to put 'display: inline-block' on you're <a> tags end center them with margin: 0 auto.
- On your 'plano-container' you use a margin-left: -40. It looks like this pulls the div out of the middle because you've already used flexbox, space-around on the parent.
- I think it's a good idea to use English in all you're documents and naming so that it is easier for other people to understand it.
Great job! Hope this feedback helps you a little bit further.
Marked as helpful
0 - Atul Yadav• 135
@titancode25
Submitted
Hey,
This was really awesome project as I was learning Grid CSS, Simple not much issues in this! Hope you love this content Developed by me!
rikvandersar• 145@RikvanderSar
Posted
Hey man. Nice job!
Just wanted to give you some quick feedback. In the tablet / smartphone view you have some horizontal scrolling. It seems some content is overflowing. And you got some accessability issues in your markup that might need to be adressed.
0 - rikvandersar• 145
@RikvanderSar
Submitted
Some tips and suggestions to further style the form elements would be great. I'd also would appreciate feedback on the readability of the code.
rikvandersar• 145@RikvanderSar
Posted
Hi Matt,
Thank you for this helpfull feedback, really appreciate it! I'm definitly learning a lot from this project.
I'll get back at it with your feedback and will post a update soon!
0 @Abduwaasi
Submitted
Hi, this is my first project on frontend mentor, feel free to review my code, i will be glad to hear your view about it...thanks in advance.
rikvandersar• 145@RikvanderSar
Posted
If you give your body a background image and have a child element for your card the card will lay on top of your background image. But you have to set a height and width for you body to display to full image. I'd go with height: 100vh
Marked as helpful
0@Abduwaasi
Submitted
Hi, this is my first project on frontend mentor, feel free to review my code, i will be glad to hear your view about it...thanks in advance.
rikvandersar• 145@RikvanderSar
Posted
Hi Adebayo,
Look good to me! If I compare your version with the design it looks like the inactive and active state of the links are vice versa. And I wonder why you've used a psuedo element for the background image. Wouldn't a background image on the body work as well and be less code?
Marked as helpful
1
