
Amobi Jonathan Chukwudi
@JonathanthedeveloperAll comments
- @pastyenrique@Jonathanthedeveloper
Hey Enrigue, Your challenge looks excellent, Kudos to that but, I noticed that you failed to add a closing HTML tag and with that, you may end up with unexpected issues with your webpage. However, excluding a closing tag will not cause error messages or crashes, so they technically are not an absolute requirement. But, this does not mean you should exclude them.
Marked as helpful - @welangaieric@Jonathanthedeveloper
Hello Eric, your solution looks great except for the fact that it isn't interactive.
In case you're wondering what Event listener could be used you can use the
keyup event listener
here's a sample solution code
// dom elements const inputs= document.querySelectorAll('input'); const outputs = document.querySelectorAll('output'); inputs.forEach(function(input, index){ input.addEventListener("keyup", function('keyup', function(){ if(validation code === true) { outputs[index].textcontent = input.value; } }) ) });
Marked as helpful - @shalash23@Jonathanthedeveloper
Hey shalash23, Your overall solution looks great.
However, you didn't save the total for each person, you are supposed to save the total of every calculation until the reset button is pressed please, do well to look into it.
- @loicsolbes@Jonathanthedeveloper
Hello Soles, Whilst your solution looked great and responsive I've noticed that has an overflow-X maybe you could try changing your container's max-width to 80% or you could also try setting the body's overflow to hidden. Thank you
Marked as helpful - @davidicabello@Jonathanthedeveloper
Hello David, Your Solution looks great, however, you should that the following into account
-
your
div.productDescription
needs some more padding 30px should be great -
your
button.addToCartButton
needs some resizing as it's too high, and the top margin seems to be too high -
you should also add a line-height to your
<p>...</p>
say 1.2 -
Then on mobile view the picture on the left seems to be distorted, maybe you could try changing the
<img>
src on the mobile view to the image provided for mobile devices.
But overall your solution looks great
Marked as helpful -
- @Konstantinchrist@Jonathanthedeveloper
Hello Konstatin, your solution to this challenge was great and awesome, very responsive and that's really cool, However Instead of putting the required keyword on the HTML input tags. you could use some form validation using javascript to verify that an input was filled and if not, you then display its dedicated error messages, also you should also add some box shadow to the div.special-offer and button attached below is a code that might help you understand what I mean
inputs.forEach( input => { // check if the input is empty })
Marked as helpful - @SebastianLaka@Jonathanthedeveloper
Hello Sebastian, You did excellent work on your rating component, however, it would be best if you validated that a button has been clicked before displaying the thank you message. I added a code below to help you with that
submitButton.addEventListener("click", () => { if (/* check if user has click on a rating*/ ){ secondCard.classList.remove("hidden"); firstCard.style.display = "none"; } else { // prompt user to click on a rating } });
Marked as helpful - @NabilWasir@Jonathanthedeveloper
Hey Nabil, whilst your solution looked great, I however noticed that you used px and now your solution isn't fully responsive on some mobiles changing you width units to % percentages would go a long way
example you could do
.main-container { width: 60%; }
- @marekbrze@Jonathanthedeveloper
Hey Marek, I noticed that you used a lot of divs . so you can refactor your code to
<div class="cardContent"> <h1>Improve your front-end skills by building projects</h1> <p> Scan the QR code to visit Frontend Mentor and take your coding skills to the next level </p> </div>
instead of
<div class="cardContent"> <h1>Improve your front-end skills by building projects</h1> <div class="cardContent"> <p> Scan the QR code to visit Frontend Mentor and take your coding skills to the next level </p> </div> </div>
You
Marked as helpful - @BennyBenV@Jonathanthedeveloper
Benjamin nice work you did there. however, I noticed your solution wasn't responsive even though you used flex on your div#main, try adding this to help improve responsivity
#main { flex-direction: row; } @media (max-width: $width ){ #main { flex-direction: column; /* add more styles for your solution*/ } }
replace $width with the screen width you want to make it responsive at e.g 800px Thank you
Marked as helpful - @MicMond01@Jonathanthedeveloper
Nice work you did there Micheal. Notice how your div.container is small on mobile devices, make it have a total width of 90% of the view width, then try reducing the font size of your h3 of smaller devices, and lastly change the let keyword you used while targeting your dom elements in your script.js to const as the dom elements are static, the only thing that changes it their values. Thank you.
- @KaydenGiang2512@Jonathanthedeveloper
I noticed that you picture was aligned to the top right, Which isn't supposed to be so.
I think this would solve it
img { width: #px; height: #px; position: absolute; left: #px; }