@jessegood
@Joatancarlos
Submitted
In this project I was able to practice concepts of regular expressions and apply Sass. I feel like I need to improve these techniques and make my code cleaner.
What tips would you give me to improve validation, to separate the numbers into groups of 4 and to make the Sass code cleaner?
const validation = {
name: /\d/g,
number: /\D/g,
}
@jessegood
Posted
Nice solution!
Here are some tips to improve your code.
To improve validation:
It seems you are performing validation on the keyup event. However, the form can still be submitted even if the form is not filled out. I would suggest listening to the submit event, perform validation and only allow submitting the form when there are no errors.
I think learning the built in JavaScript Validation API would be good. For example, you can set the required attribute to input forms and then just check input.validity.valueMissing to see if the field is blank.
Putting numbers into groups of 4:
Here is the function I use. You could listen to the input event on your input field and then format the input using the function below.
function formatCreditCardNumber(num) {
num = num.split(" ").join("");
let arr = num.split("");
let formattedNum = [];
if (arr.length < 4) return num;
for (let i = 0; i < arr.length; i++) {
if (i !== 0 && i % 4 === 0) {
formattedNum.push(" ");
}
formattedNum.push(arr[i]);
}
return formattedNum.join("");
}
For improving your SASS, I highly recommend looking into nesting to make your CSS more compact and readable: Here is a tiny example using your code. However,
.container {
.aside {
width: 100%;
height: 40vh;
background-repeat: no-repeat;
background-size: cover;
.front-card {
position: absolute;
top: 70%;
left: 10%;
z-index: 1;
}
}
}
The above compiles to:
.container .aside {
width: 100%;
height: 40vh;
background-repeat: no-repeat;
background-size: cover;
}
.container .aside .front-card {
position: absolute;
top: 70%;
left: 10%;
z-index: 1;
}
Also, to space our your numbers, use font-variant-numeric: tabular-nums;!
@KaanKaramese
Submitted
I would like to get feedback for validation form since my code is kinda sloppy. Any feedback regarding accessibility and responsiveness (especially for credit card divs) is also welcome.
@jessegood
Posted
Hi Kaan
Nice solution! Here is some feedback, I hope it helps:
It appears you did all the validation by yourself, but I would highly recommend looking into the validation API built-in to browsers. Basically let the browser do the checking for you! For example, to check for empty input, you can do something like the following:
required attribute to your input like this:
<input required ... >var validity = input.validity;
if (validity.valueMissing) // Do something
Concerning responsiveness. I see a media query for max-width: 372px and then the next one is min-width: 500px so anything in between that is not accounted for. As a quick test, try resizing your browser and see how the layout changes. Also, I would recommend setting a min-width on the cards so they do not become too small.
Try setting the card number to all 1s like this: 1111 1111 1111 1111.
You will see that the width is off. To fix this set the following css property font-variant-numeric: tabular-nums;.
Marked as helpful
@arey-dev
Submitted
This is my first Javascript Challenge. My knowledge in javascript is beginner level, so any feedback and advice will be greatly appreciated.
@jessegood
Posted
Hi arey
I just submitted a solution for the same problem. Here is some feedback I hope it helps!
The <div> element is a block element and is not allowed inside <label> elements. I assume you meant to use the numbers as labels? In that case you could just remove the <div> from them and they would become the label.
You had two media queries, one for 400px and 360px. I wasn't sure why you split them up as they seem very close.
I see that you built the Thank You part in JavaScript. One easier way might be to use display: none and then just set it to display when you want to show it (and set the other part to display: none).
Instead of adding event listeners on each element you want. Another idea would be to just add an event listener on the document and then just only react to click events when a user clicks a radio-input or the button. You could also remove type="submit" from your button so it does not act like a submit button.
Also, doesn't affect your case, but as a JavaScript best practice, always use the === (the triple equals).
Marked as helpful
@TanyaJunior
Submitted
l will really appreciate feedback
@jessegood
Posted
Hi Tanya
Welcome to frontend mentor. Here is some feedback, hopefully it will come in handy.
<main> element helps for accessibility (as they know where the main to go for the main portion of the document). Also, semantically also this is preferred.style-guide.md file? It provides information on fonts and colors that should be used.<img> is missing an alt attribute. Also, type is not a valid attribute for this element.<div>.
For example you could get away with the following structure:Also, In your CSS, you set the font-family, etc. multiple times. CSS has a notion of "inheritance". If you look at other examples, the font is often times set only once in the body as this is inherited by all other elements.
Hopefully that helps.
@sadhap
Submitted
@jessegood
Posted
Here are a few points I saw. Hopefully this helps.
(1) Looks like you mispelled card-img-containere which is causing some of the styles not to work.
(2) I would recommend not wrapping everything in a div and it will cut down on how much styling you have to use.
(3) The other comment already addressed it, but using a main landmark for accessibility is also a plus.
