Newsletter Sign Up using sass and css flexbox

Community feedback

• 

  P

  Rupali•890

  @rupali317

  Posted 11 months ago

  Hello @danielbasilio

  Front-end mentor team requested me to review your code and provide feedback since I am following the Javascript learning path and I have also finished this particular challenge. It is my pleasure reviewing your code and offering suggestions for the code improvement:

  Your HTML code is semantic (especially how you used the address tag and the picture tag).

  My suggestions:

  • In the browser, when I altered the font settings to large, I noticed that some of the font sizes did not scale accordingly. This is because you used px as font size. I suggest you use rem unit to make the fonts scalable otherwise the text will not be accessible to those who rely on bigger font size via browser.

  • You used an inline style of display:block and display:none. You can eliminate the usage of this inline style and instead toggle the appearance of hide-error class. element.classList.toggle("hide-error") will achieve it. Classes have lower specificity than inline styles.

  • The javascript functions like displayError and enableButton can be renamed as toggleInlineError and toggleButtonState respectively. The former names did not convey the actual functionality and hence it becomes difficult to understand the code at an initial glance.

  • When I typed the correct email, I expected the disabled state of the button to go away. But it did not do so. It is a bug. In your "displayError" function, you also need to handle the state of the button. So based on that, please rename the function accordingly so that the purpose of the function is conveyed.

  • The variables in the javascript are well named. One point where you can improve the performance is not to always use document.querySelector(). I want to highlight the document part since it will check from the entire document. It is doing so 6 times since you declared 6 variables. You can improve it in this way:

  const $formSignUp = document.querySelector('[data-js="form-sign-up"]');
  const $emailField = $formSignUp.querySelector('[data-js="form-email-field"]');
  const $btnSubmit = $formSignUp.querySelector('[data-js="form-btn-submit"]');
  const $modal = document.querySelector('[data-js="modal"]');
  const $btnModal = $modal.querySelector('[data-js="modal-button"]');
  const $errorMessage = $formSignUp.querySelector('[data-js="error-message"]')
  

  In the above example when I want to select an email field, I do not have to use document again since I have already searched sign up form. To select the email input, I can search it from the sign up form rather than combing the document again.

  • The modal component's email should be based on what the user has typed on the email input field. Currently it is ash@loremcompany.com. Also, it is nice to see that you have made the email address as a link. However, clicking on that email is showing a 404. I think you wanted to open an email. To do so, you need change it as: <a href="mailto:ash@loremcompany.com" title="Confirm your email" class="link">ash@loremcompany.com</a>

  00