base-apparel-coming-soon-master

Community feedback

• 

  solitary_coder•1,000

  @kabir-afkPosted about 1 year ago

  Hey, good job on completing the challenge , so I checked out your site and came to realize that , your error handling is visible for a short span of time and then suddenly everything resets . . . . to prevent that from happening you could have done something like

  const form = document.querySelector("form");
  form.addeventlistener('submit',(event)=>{
  event.preventDefault();
  //rest of the styling
  })
  

  Also in-order to use this , you should set your button type = submit, because technically you are submitting your form . . . .

  other thing I noticed was that you are inserting your error-images in your html, it is absolutely ok to do that in short run since we all are starting out , but in the long run when you have multiple input fields and error-messages and error-images to handle, it doesn't turn put to be the best method , try looking up for HTML Insertion mehods and other insertion methods in JS like insertAdjacentHTML,insertAdjacentElement,etc etc . . .

  Ok so the final correction I would like to add will be DRY -Donot Repeat Yourself

  On reviewing your JS code I found the if-else statements to be really repetitive , I know the code is like uber small but hey , it is about following the best practices right ?, so instead of this

  if(reg.test(email)){
      document.querySelector('.text-error').innerText='Thanks for your information!';
      document.querySelector('.text-error').style.visibility='visible';(🙄)
      document.querySelector('.error-icon').style.opacity='0';
      document.querySelector('.text-error').style.color='green';
  }
  else{
      document.querySelector('.text-error').innerText='Please provide us your valid email';
      document.querySelector('.text-error').style.visibility='visible';( cant understand what was the point of adding it to the if-else block if its visible in both 🙄, too lazy to find the element on my own 🥱)
      document.querySelector('.error-icon').style.opacity='1';
      document.querySelector('.text-error').style.color=inherit;
  }
  

  something like this would have been better

      document.querySelector('.text-error').innerText= reg.test(email) ? 'Thanks for your information!' :  'Please provide us your valid email';
      document.querySelector('.error-icon').style.opacity=reg.test(email) ? '0' : '1';
      document.querySelector('.text-error').style.color= reg.test(email) ? 'green' : 'inherit' ;
  

  It makes for a shorter code , easier to read and easier to maintain, these are called ternary operators btw , look up on them as well . . .

  Last but not the least , instead of adding individual styling on each DOM element using .style , you can specify your class in CSS and access it using the classList method . . . this'll also allow you to add, remove, toggle, a class when and however needed , you can also check if a certain class is present using classList.contains('class-name') method.

  Why you may ask ?

  Well a time will come when you'll add a theme switcher to your webpage , then , if you were to access each element individually, it'll make for a lame code , what you can do is you can change the values of all the variables of dark colors to light ones or vice versa in your CSS and wrap them around in a single class. When you have to change theme on click or any other event you can simply write

  document.body.classList.toggle('whatever-dark/light class name you have')

  As far as the JS Regex is concerned it looks and works right for the most part, not very good in it (ask gpt or net for an even more optimized version)

  That was all , hope you found it helpful and keep the grind . . .🥂🥂

  Marked as helpful

  1