Hello I recently finished my first project Qr-code-component, I hope you will like, I would like any advices to be better and learn each day all new technics to be a better frontend developer.
Amobi Jonathan Chukwudi
@Jonathanthedeveloper
All comments
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
0- Welangai Eric• 160
@welangaieric
Submitted
there is a lot to be fixed. but i can say i tried
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
0 - shalash23• 280
@shalash23
Submitted
I would really any advice about refactoring the JavaScript code. Is there room for improvement here?
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.
0 - soles• 40
@loicsolbes
Submitted
Hi ! I have just finish the challange and I have a few questions. Is this a responsive design ? Which parts of my code can I improve? Do I have a good use of em and rem ? Thank you for your help
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
0 - David Iván Cabello• 110
@davidicabello
Submitted
Any feedback welcome. I have to look for some stuff online to resolve this!. Thanks!
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
1 -
- Konstantin Christ• 450
@Konstantinchrist
Submitted
Hi guys, I hope you're doing great. I just didn't know how to implement the errrors if you leave something blank. Maybe someone can tell me :). Thanks for the support!
Konstantin
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
0 - Sebastian• 220
@SebastianLaka
Submitted
Hi it's my first interactive project with JS. All is in repo at my github.
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
0 - Nabil Wasir• 680
@NabilWasir
Submitted
Don't forget to tell me if something is wrong in my code or if I can improve my code and give your opinion/feedback.
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%; }
0 - Marek Brzezinski• 40
@marekbrze
Submitted
I would like to know if my layout decisions were good. Especially regarding margins and positioning of the elements.
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
1 - Benjamin• 120
@BennyBenV
Submitted
Hello ! Here is my solution to this challenge ! I enjoyed doing this challenge and learn how to do responsive forms.
Feedbacks would be very helpful to me!
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
0 - Okudero Michael• 350
@MicMond01
Submitted
This is my first API challenge. Kindly leave a comment in case there's an improvement to be made.
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.
1 Definitely one of the harder challenges I encountered on Frontend Mentor, especially with the difficulties of building a multi-layout navbar (since it's my first time), so if anyone has a more concise solution or know any useful knowledge on how to achieve the same outcome with a more efficient approach, please don't hesitate to leave a comment and help me out :D Thank you very much!
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; }
0