Skip to content
OverviewSolutions
6
Comments
3
Ricky Hewitt
@rickyhewitt

All comments

  • Ricky Hewitt•90
    @rickyhewittPosted over 1 year ago

    Unfortunately it seems like it is broken in both desktop & mobile (screenshot)

    The star icon can be fixed by updating the path to:

    ./images/icon-star.svg

    The width of the card in the mobile version will need to be increased:

    margin: 5% auto 0 auto;
width: 320px;

    While the margin between the buttons (.b1, .b2, .b3, .b4, .b5) on desktop will need to be decreased slightly to fit them all on one line as in the original design (or decrease button padding slightly).

    As the CSS for the .b1, .b2, .b3, .b4, .b5 is being duplicated, you only need one class for these.

    The submission leads to a 404, so unfortunately that also needs to be fixed.

    0
  • Ricky Hewitt•90
    @rickyhewittPosted over 1 year ago

    Great work!

    One thing I would note is that the desktop design seems to be incorrect, it is perhaps only using the mobile one.

    The other thing I noticed is that you could avoid duplication in your javascript for eventhandlers.

    For example, your existing code:

    document.querySelector("#name").addEventListener("keyup", checkErrorsOnPress);
document.querySelector("#number").addEventListener("keyup", checkErrorsOnPress);
document.querySelector("#mm").addEventListener("keyup", checkErrorsOnPress);
document.querySelector("#yy").addEventListener("keyup", checkErrorsOnPress);
document.querySelector("#cvc").addEventListener("keyup", checkErrorsOnPress);

    Could be replaced with either event delegation or a loop.

    const elements = document.getElementsByTagName("input");
for (let i = 0; i < elements.length; i++) {
elements[i].addEventListener("keyup", checkErrorsOnPress);
}
    0
  • Ricky Hewitt•90
    @rickyhewittPosted over 1 year ago

    The header in the mobile version appears to be missing.

    I would also recommend against using position: absolute, but instead using CSS grid or flexbox.

    0