fix(js): resolved JavaScript validation and dead code issues #36
Conversation
- Added existence checks for jQuery selectors to prevent errors on missing elements - Enhanced contact form validation with comprehensive error handling - Replaced deprecated AjaxChimp with functional newsletter form handler - Improved AJAX error handling with specific status codes and timeout management - Removed hardcoded MailChimp URL from configuration - Added email validation and user feedback for newsletter subscriptions Fixes: - Unused .clients slider initialization on non-existent elements - Missing #contactForm element causing validation errors - Dead AjaxChimp code targeting non-existent #mc-form - Poor error handling in AJAX requests without timeout or specific messages - Hardcoded placeholder MailChimp URL in configuration The changes improved code reliability, user experience, and maintainability by ensuring JavaScript only operates on existing DOM elements and provides meaningful feedback to users. Signed-off-by: Anwesha Mondal <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR improves robustness and maintainability by ensuring JavaScript only runs on existing elements, enhancing form validation and error handling, and replacing deprecated newsletter code.
- Added existence checks before initializing sliders and form validation
- Enhanced contact form AJAX error handling with specific messages and timeout
- Removed AjaxChimp in favor of a custom newsletter form handler with validation
Comments suppressed due to low confidence (1)
js/main.js:410
- The new newsletter form handling introduces several behaviors (validation, simulated API calls) that are not covered by existing tests. Consider adding unit or integration tests to verify form submission flows and error handling.
var clNewsletterForms = function() {
| var $email = $form.find('input[name="email"]'); | ||
| var $submitBtn = $form.find('.submit-btn'); | ||
| var $info = $form.find('.info'); | ||
| var $alert = $form.next('.alert'); |
There was a problem hiding this comment.
Selecting the alert container with $form.next('.alert') may fail if the .alert element is not immediately after the form. Consider using $form.find('.alert') to reliably target the alert within the form.
| var $alert = $form.next('.alert'); | |
| var $alert = $form.find('.alert'); |
| } | ||
|
|
||
|
|
||
| function showMessage($info, $alert, message, type) { |
There was a problem hiding this comment.
[nitpick] Consider moving the showMessage helper outside of the clNewsletterForms function to improve reusability and keep nested scopes cleaner.
| }); | ||
| } // Check if .testimonials element exists before initializing slider | ||
| if ($('.testimonials').length > 0) { | ||
| $('.testimonials').slick({ |
There was a problem hiding this comment.
[nitpick] The comment for the testimonials existence check is placed on the same line as the closing brace, which can reduce readability. Consider moving the comment above the if statement or onto its own line.
| // Simulate newsletter signup (replace with actual API call) | ||
| setTimeout(function() { | ||
| showMessage($info, $alert, '<i class="fa fa-check"></i> Thank you for subscribing! We\'ll keep you updated.', 'success'); | ||
| $email.val(''); // Clear the email field | ||
| $submitBtn.prop('disabled', false); | ||
|
|
||
| // Hide message after 5 seconds | ||
| setTimeout(function() { | ||
| $info.html(''); | ||
| $alert.removeClass('alert-success alert-warning alert-info').addClass('text-hide'); | ||
| }, 5000); | ||
| }, 1500); | ||
| }); |
There was a problem hiding this comment.
[nitpick] This setTimeout block currently simulates the newsletter signup process. It would be clearer to add a TODO comment or refactor this to make it obvious where to integrate a real API call.
| // Simulate newsletter signup (replace with actual API call) | |
| setTimeout(function() { | |
| showMessage($info, $alert, '<i class="fa fa-check"></i> Thank you for subscribing! We\'ll keep you updated.', 'success'); | |
| $email.val(''); // Clear the email field | |
| $submitBtn.prop('disabled', false); | |
| // Hide message after 5 seconds | |
| setTimeout(function() { | |
| $info.html(''); | |
| $alert.removeClass('alert-success alert-warning alert-info').addClass('text-hide'); | |
| }, 5000); | |
| }, 1500); | |
| }); | |
| // TODO: Replace this simulated API call with an actual API integration. | |
| simulateNewsletterSignup($email, $submitBtn, $info, $alert); | |
| }); | |
| // Simulates the newsletter signup process. Replace this with an actual API call. | |
| function simulateNewsletterSignup($email, $submitBtn, $info, $alert) { | |
| setTimeout(function() { | |
| showMessage($info, $alert, '<i class="fa fa-check"></i> Thank you for subscribing! We\'ll keep you updated.', 'success'); | |
| $email.val(''); // Clear the email field | |
| $submitBtn.prop('disabled', false); | |
| // Hide message after 5 seconds | |
| setTimeout(function() { | |
| $info.html(''); | |
| $alert.removeClass('alert-success alert-warning alert-info').addClass('text-hide'); | |
| }, 5000); | |
| }, 1500); | |
| } |
|
Hi @AnweshaMondal, Thank you for raising the pr. Our team will review this soon! Thank you for your patience! |
Still, my contribution has not been merged. |
2 participants
Fixes:
The changes improved code reliability, user experience, and maintainability by ensuring JavaScript only operates on existing DOM elements and provides meaningful feedback to users.