add task solution #1287
add task solution #1287
Conversation
VasylPylypchynets
commented
Oct 15, 2024
VasylPylypchynets
commented
- DEMO LINK
There was a problem hiding this comment.
Great job on the code! 😊 However, there are a few areas that need a bit of tweaking to improve the performance and maintainability. Please consider the suggestions provided, such as checking if an attribute exists before manipulating it, using the appropriate functions to capitalize strings, and using a forEach loop for better readability. Keep up the good work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/scripts/main.js
Outdated
| for (let i = 0; i < fieldText.length; i++) { | ||
| const input = fieldText[i]; | ||
| const label = document.createElement('label'); | ||
| const nameInput = splitCamelCase(input.getAttribute('name')); | ||
| const capitalizedWords = capitalizeWords(nameInput); | ||
| const parentInput = input.parentNode; | ||
|
|
||
| label.classList.add('field-label'); | ||
| label.setAttribute('for', input.getAttribute('name')); | ||
| label.textContent = nameInput.toLocaleUpperCase(); | ||
|
|
||
| input.setAttribute('placeholder', capitalizedWords); | ||
|
|
||
| parentInput.insertBefore(label, input); | ||
| } |
There was a problem hiding this comment.
Consider using a forEach loop instead of a for loop. This would make your code more readable and less error-prone, especially when working with arrays or array-like objects. Here is how you could do it: fieldText.forEach((input) => { ... });
src/scripts/main.js
Outdated
|
|
||
| label.classList.add('field-label'); | ||
| label.setAttribute('for', input.getAttribute('name')); | ||
| label.textContent = nameInput.toLocaleUpperCase(); |
There was a problem hiding this comment.
It seems like you're using toLocaleUpperCase() to convert the entire string to uppercase. However, you've already created a capitalizeWords function which capitalizes the first letter of each word. If you want to capitalize the entire string, you could just use toUpperCase(). If you want to only capitalize the first letter of each word, you should use your capitalizeWords function instead.
src/scripts/main.js
Outdated
| for (let i = 0; i < fieldText.length; i++) { | ||
| const input = fieldText[i]; | ||
| const label = document.createElement('label'); | ||
| const nameInput = splitCamelCase(input.getAttribute('name')); |
There was a problem hiding this comment.
It's a good practice to check if an attribute exists before trying to manipulate it. In this case, you could add a condition to check if input.getAttribute('name') is not null before calling splitCamelCase.
|
|
||
| input.setAttribute('placeholder', capitalizedWords); | ||
|
|
||
| parentInput.insertBefore(label, input); |
There was a problem hiding this comment.
You are inserting the label before the input field. Usually, it's more common to have the label before the input field in the HTML structure for better accessibility. If you want to change the visual order, consider using CSS.
There was a problem hiding this comment.
Great job on your code! 👏 However, there are a few areas that need some improvements for better functionality and consistency. Please take a look at the comments and make the necessary adjustments. Keep up the good work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/scripts/main.js
Outdated
| function splitCamelCase(str) { | ||
| return str.replace(/([a-z])([A-Z])/g, '$1 $2'); | ||
| } |
There was a problem hiding this comment.
The splitCamelCase function is a good use of regular expressions to split camel case words. However, it doesn't handle cases where the string starts with a capital letter. Consider adding a condition to handle this case.
src/scripts/main.js
Outdated
| function capitalizeWords(str) { | ||
| return str | ||
| .split(' ') | ||
| .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) | ||
| .join(' '); | ||
| } |
There was a problem hiding this comment.
The capitalizeWords function is a good use of the map function to capitalize each word in a string. However, it doesn't handle cases where the string is null or undefined. Consider adding a condition to handle these cases.
src/scripts/main.js
Outdated
| fieldText.forEach((input) => { | ||
| const label = document.createElement('label'); | ||
| const nameInput = input.getAttribute('name') | ||
| ? splitCamelCase(input.getAttribute('name')) | ||
| : null; | ||
| const capitalizedWords = capitalizeWords(nameInput); | ||
| const parentInput = input.parentNode; | ||
|
|
||
| label.classList.add('field-label'); | ||
| label.setAttribute('for', input.getAttribute('name')); | ||
| label.textContent = nameInput.toUpperCase(); | ||
|
|
||
| input.setAttribute('placeholder', capitalizedWords); | ||
|
|
||
| parentInput.insertBefore(label, input); | ||
| }); |
There was a problem hiding this comment.
In the forEach loop where you create a label for each input field, you're getting the 'name' attribute of the input twice (line 19 and 26). Consider storing it in a variable to avoid duplicate operations. Also, you're setting the 'placeholder' attribute of the input to the capitalized words (line 29), but then you're setting the text content of the label to the uppercased name (line 27). This is inconsistent. Consider making them both capitalized for consistency.
There was a problem hiding this comment.
Great job on your code! It is generally well-written and follows good practices. However, there are some areas that could be improved for better readability and performance. Please review the comments and make necessary adjustments. Remember, continuous improvement is key to becoming a great developer. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const fieldText = document.querySelectorAll('.field-text'); | ||
|
|
||
| function splitCamelCase(str) { | ||
| let string = str; |
There was a problem hiding this comment.
There is no need to create a new variable string that is a copy of str. You can directly manipulate str in your function. This would make your code cleaner and more efficient.
| function splitCamelCase(str) { | ||
| let string = str; | ||
|
|
||
| if (string.charAt(0).toUpperCase() === string.charAt(0)) { |
There was a problem hiding this comment.
This line checks if the first character of the string is uppercase. This is not wrong, but it might be more clear to other developers if you use a regular expression to check this. For example, you can use if (/^[A-Z]/.test(string)) to achieve the same result.
|
|
||
| label.classList.add('field-label'); | ||
| label.setAttribute('for', input.getAttribute(InputName)); | ||
| label.textContent = nameInput.toUpperCase(); |
There was a problem hiding this comment.
You are setting the label's text content to the uppercase version of nameInput. However, you have already created a capitalized version of nameInput in the capitalizedWords variable. It would be more efficient to use capitalizedWords here instead of calling toUpperCase() again.
| label.setAttribute('for', input.getAttribute(InputName)); | ||
| label.textContent = nameInput.toUpperCase(); | ||
|
|
||
| input.setAttribute('placeholder', capitalizedWords); |
There was a problem hiding this comment.
In this line, you are setting the input's placeholder to the capitalized version of nameInput. However, if nameInput is null, capitalizeWords will return an empty string, and your input will have an empty placeholder. You should add a condition to check if nameInput is not null before setting the placeholder.
