Latest solutions
Latest comments
- @Joatancarlos@jessegood
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 thesubmit
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 checkinput.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@jessegood
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:
- Add the
required
attribute to your input like this: <input required ... > - In Javascript, you can check the input like this:
var validity = input.validity; if (validity.valueMissing) // Do something
- I recommend reading this article about it. Let me know if you have any questions.
- Add the
-
Concerning responsiveness. I see a media query for
max-width: 372px
and then the next one ismin-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 amin-width
on the cards so they do not become too small. -
Try setting the card number to all
1
s like this: 1111 1111 1111 1111. You will see that the width is off. To fix this set the following css propertyfont-variant-numeric: tabular-nums;
.
Marked as helpful -
- P@arey-dev@jessegood
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
and360px
. 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 todisplay: 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 removetype="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@jessegood
Hi Tanya
Welcome to frontend mentor. Here is some feedback, hopefully it will come in handy.
- Wrapping everything in a
<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. - Did you look at the
style-guide.md
file? It provides information on fonts and colors that should be used. - The
<img>
is missing analt
attribute. Also,type
is not a valid attribute for this element. - Try minimizing the use of
<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.
- Wrapping everything in a
- @sadhap@jessegood
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.