@MasterKrab
Submitted
I am just a beginner and would appreciate any feedback.
Do I have good practices? What errors do I have? How could I improve my code?
Thanks <3
Looking to hire developers?
@iamjmitch
@MasterKrab
Submitted
I am just a beginner and would appreciate any feedback.
Do I have good practices? What errors do I have? How could I improve my code?
Thanks <3
@iamjmitch
Posted
Hi Masterkrab,
Some nice work here but can I ask what lead you to choose using createElement
and appending it to the page? Just seems like a really over complicated way of implementing that error message.
@mohamed1maghraby-div
Submitted
any comment related to React.js will be very helpful
@iamjmitch
Posted
Hey mate nice looking website. Really nice folder structure and good use of components.
I see in your section2 component, you just have react render the card component 3 times however as the info inside the card is hardcoded, it displays the same reviewer and comment 3 times.
If you were to change the card to accept props, you could still have the same card component for all 3 however you could pass it seperate info for the img
, h4
and p
tags and have it render as 3 seperate reviews like in the design guide.
Suggestion on HTML structure & CSS clean code is highly appreciated.
@iamjmitch
Posted
Hey mate looks good. Code is nice and clean.
One suggestion though is you have CSS in both an external stylesheet and internally inside your HTML file. While this isn't a big issue on smaller projects like this, as your projects get bigger, it will make your coding experience easier if you pick one universal way of maintaining your CSS
@Ducknaro
Submitted
HI guys, can you tell me whether or not there is anything wrong with my code? any feedback would be much appreciated. thanks
@iamjmitch
Posted
Hi Luca,
Really nice work on this, looks really good.
Nothing wrong with your code, I would suggest though that you only us one H1
tag on the page. You've currently got 3 so I would suggest using a H2
and H3
aswell. If all the headings are H1
then none of them are really H1
if you know what I mean. Keeping it won't effect anything as such but it is technically semantically incorrect
Hello, this is my first project. I hope you guys can give me feedback so I can improve my skills later on
@iamjmitch
Posted
Hi,
Interesting use of the hr
tag there to make the dividing line. Would have never thought of it.
With your code, you've used the h1
tag alot where not necessarily needed. Generally you only want to use 1 h1
tag per page or section
@lamaolo
Submitted
I need to do it responsive, but first I should refactorize the code. Definitely it would have been a better idea do it mobile-first.
@iamjmitch
Posted
Really nice animation on this!
Just a thought with the different tokens when it comes to refactoring. all 3 are essentially the same thing just with different images and colors. You could potentially just create one token component and pass it in the img url and colors as props. This would essentially turn 3 separate components into one.
@sandeepPainkra
Submitted
This is my second frontend project..and i have tried so hard to make this.. using html,css and javascript..
@iamjmitch
Posted
Nice work so far!
couple of suggestions with the "latest articles" cards. Have a play with the box-shadow on them and see if you can make them a bit softer. I like that they scale on hover but I think it would take it to the next level if you added some sort of transition in your CSS for them
@Al11o
Submitted
I don't succeed the size of the card
@iamjmitch
Posted
Hi, You're almost there with your CSS. To fix you issue of the gap at the bottom, try the following:
Remove
.card #content{ top: -52.5px; }
Add
.card #content img { margin-top: -50px; }
@Tommy394
Submitted
Hi! I'm still confused when is right time to use px, em, or rem, so I'm mostly use rem for the length value, is that the right thing to do?.
And any suggestion on the other CSS technologies that I can use for this challenge?
I'm new to the front end development, I don't know if this is the best approach to this challenge, so a feedback would meant a lot to me.
Thanks!
@iamjmitch
Posted
Hi, in terms of when to use rem
vs em
vs px
vs %
, for your own personal projects, use what ever you're comfortable with. In a professional environment, have a read of this article https://engageinteractive.co.uk/blog/em-vs-rem-vs-px
With the CSS there's nothing wrong with using SASS or something similar. How youve done it is fine as the CSS required is quite short however once you get to bigger projects you'll make your coding faster by using some sort of CSS pre-processor
@ksnitsky
Submitted
Any feedback would be nice
@iamjmitch
Posted
Nice work. I am genuinely curious how you came to name one of your classes "poop" though
@firhanram
Submitted
I used many grids, is this an effective way ?
@iamjmitch
Posted
I think you have used grid quite effectively and its actually quite a nice approach to solving the task. Well done
I would like some feedback please.
@iamjmitch
Posted
Nice looking work mate, well done. Really like how you've made it so your classes are reusable, that's a good practice to keep up.
Have a look on mobile though, your attribution statement is randomly halfway down the screen and not at the bottom as intended. Fox that and you'll be golden
I'm just completely lost in the javascript-form validation. And I have no idea how to clear the input. I've tried .reset()
, I tried resetting the values with (something similar to) email[0].value = ''
. Would anyone know a good source for more information? I tried a zillion YouTube-video's, but noone resets the value and I think that is why I'm running into errors?
As for the css & html: this was fairly logic and no questions here really.
@iamjmitch
Posted
Hi there,
Ill do my best to try and explain some stuff
To answer your reset question, try form.reset()
. this worked for me.
the other issue you are having is that the JS is not actually capturing the value of the input. const email = document.getElementById('email').value.trim()
is running at page load, not when you hit the notify button so the inputted email address is never actually being captured and passed to the checkInputs()
function for validation. Try declaring email on load const email = document.getElementById('email')
and then inside your eventListener have something that looks like
form.addEventListener('submit', (e) => {
e.preventDefault();
var inputtedValue = email.value.trim();
checkInputs(inputtedValue);
})
this will make sure that the most up to date value is always passed to your check function when the user hits the notify button
Then with your actual check function, if(!email)
is checking to see if the variable doesn't exist which, as you are declaring it on page load, email
will never not exist and the first block of code will never run. What you want to do is check whether that value is an empty string or ""
function checkInputs(inputtedValue) {
if (inputtedValue === "") {
message.classList.remove('valid')
message.classList.add('invalid')
message.innerText = 'You forgot to enter your address'
message.style.display = 'inline-block'
} else if (inputtedValue.match(emailPattern)) {
message.classList.add('valid')
message.classList.remove('invalid')
message.innerText = 'Your email address is valid'
message.style.display = 'inline-block'
} else {
message.classList.remove('valid')
message.classList.add('invalid')
message.innerText = 'Oops, something went wrong'
message.style.display = 'inline-block'
}
}
Have a look at the above modified function and see if you can work out whats going on. All the above put together should result in the functionality that your after. Hopefully this somewhat explains some stuff. Please let me know if you still dont understand and I can see how I can help
@juanma1331
Submitted
I was unable to solve the next. When decreasing viewport from desktop reviews text takes more vertical space breaking the reviews layout.
@iamjmitch
Posted
Hi mate,
Not 100% sure what your referring to. Loaded the site in mobile and it looks pretty spot on to the design. If you can elaborate some more id be happy to give advice
@ashraf-007
Submitted
Hi Guys , if there any feedback or reviews after you see the final projects , don't hesitate , i will really appreciate it
@iamjmitch
Posted
Hello there
Really nice work. Just a suggestion, I would change the following in your body tag in the CSS
Remove
height: 120vh
Add
min-height: 100vh;
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
This will help center the content on the screen and remove the white space at the bottom of the page
@kofinartey
Submitted
Hi everyone. This is my first published project since I begun learning to code. I'd appreciate it if anyone goes through my solution and suggest areas I could improve upon. Super happy to learn from y'all.
@iamjmitch
Posted
Nice work. Good job on commenting your code!
Only suggestion I have is maybe run the code though a HTML beautifier or install the plugin prettier just to clean up the white space. Definitely not a must do, nothing wrong with your code, just helps clean it up a bit visually
@Iraada
Submitted
Hey People :D
What do you think? Any suggestions on how I can improve are welcome!
I have a question: when do you use: px, em/rem, %?
Thank you
@iamjmitch
Posted
In answer of your question: when do you use: px, em/rem, %?
Best practice is supposedly to use rem
but for your own personal projects, use what ever you feel most comfortable with.
Personally I use px
and %
for container tags and px
for text.
@Cats-n-coffee
Submitted
Hi everyone! I did this challenge using plain HTML, JS and organized the styles with SCSS. I did a little bit of research/learning on how to scale SVGs and use them the proper way so they behave as expected, let me know if I should've done things differently. I believe accessibility could be a lot better, let me know what I should change first. The dynamic result list is inside a ul tag, so all rows are li tags with span tags inside (does it make sense?). Any feedback on anything is greatly appreciated, thank you!
@iamjmitch
Posted
Nice looking website. Only thing I would suggest is to add the max-width of 1440px from the style guide as when viewing the site on a 2k monitor, everything is really spread out and far apart
@jakubserwin
Submitted
If the design was made for 1440px does it look ok for widths like 2560 or 1920?
@iamjmitch
Posted
I've checked both resolutions for you and everything scales really nicely.
One issue I noticed is that your social media sprites don't load on chrome on my machine. Sprites load perfectly on Firefox and MS Edge
@shadib0797
Submitted
Any suggestions are welcome
@iamjmitch
Posted
Really clean work, I like it.
Small suggestion is you've jumped from a H1 tag to a H3 tag. Normally, you don't really want to jump like that. I would suggest changing your very first H3 tag into a H2 tag to follow correct heading hierarchy. Just remember to change in your CSS too!
@ZakariaJounir
Submitted
I would like to know if there is anything wrong or any improvement I can add to my code
@iamjmitch
Posted
Nice work!
Having a look at your code, I've noticed you use br tags a lot to vertically separate your span tags. While you can do this, I'd recommend playing around with using padding/margin in your CSS to create this gap as you will be able to customize the spacing a lot easier. You might have to set your span tags to width:100% for it to push the next item onto the next line.
@GerriEzeocha
Submitted
I think my layout skills/efficiency is getting better. As usual, open to feedback!
@iamjmitch
Posted
I cant seem to load your preview site from above however going purely off the comparison images i'd suggest vertically aligning the left side of the heading, subheading and the first card
@CrisCaGoVe
Submitted
Hello everyone, i'd like to recieve all kinds of advices and recommendations to improve my coding, thanks!
@iamjmitch
Posted
This is really good! Nice clean code, well done!
@BlakeDykes
Submitted
Learned a lot doing this, and proud of how this turned out! I'd love any constructive feedback/comments.
@iamjmitch
Posted
This is really cool man! Smooth animations.
Small suggestion I'd make is have the "show all" button have a slide in animation aswell after the last image