Skip to content
OverviewSolutions
1
Comments
29
James Mitchell
@iamjmitch

All comments

  • James Mitchell•295
    @iamjmitch

    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.

  • James Mitchell•295
    @iamjmitch

    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.

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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.

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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; }

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    Nice work. I am genuinely curious how you came to name one of your classes "poop" though

  • James Mitchell•295
    @iamjmitch

    I think you have used grid quite effectively and its actually quite a nice approach to solving the task. Well done

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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

  • James Mitchell•295
    @iamjmitch

    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.

  • James Mitchell•295
    @iamjmitch

    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