Cuong Nguyen• 70
@cuong0411
Submitted
BÌnh
@Binh2
All comments
- BÌnh• 110
@Binh2
Posted
Hey there! 👋 Here are some suggestions to help improve your code.
Congrats on completing your challenge! 🙌
I have 3 suggestions for you:
- Add a better alt attribute to your <img> tag <img alt="QR code to Frontend Mentor" />.
- Add box-shadow to your card
.card { box-shadow: 0.5em 0.5em 1em var(--clr-grayish-blue); }. You could choose a different color box-shadow if you want. - You use a <h1> tag and make it inclusively hidden, but you already use the
<title>Frontend Mentor | QR code component</title>so you don't need <h1> tag :v. Cool trick though.
If you have any questions or need further clarification, feel free to reach out to me.
You've done so well. I can't find many errors.
Happy Coding! <3 😊
2 - Talasa• 130
@TalasaDev
Submitted
Hi everyone,
this is a new solution for the News Homepage and the first junior-level challenge I've completed. I finally got to figure out how to get the dimmer effect in the main element when toggling the menu in mobile design. I add an empty <div class="dimmer"> element to the <main> and use styling to overlay it and toggle the background color with Javascript.
I will appreciate any feedback to learn about different approaches.
Happy coding!!!
BÌnh• 110@Binh2
Posted
Hey there! 👋 Here are some suggestions to help improve your code:
Congrats on completing your first junior challenge! 🙌
Answer to your question: you could set .dimmer { background: black; } and use JS to add to style attribute opacity: 0.7;. Not much different to your original way :v ...
I have 7 suggestions for you:
- Fix: Change color to match design image `h1, h4 { color: hsl(240, 100%, 5%); /* Very dark blue */ }
- Fix: You should not use <h1>, <h2>, <h3>, <h4> carelessly because "New" is not the sub-heading of "The Bright Future of Web 3.0?" so "New" can't be used with <h2> tag but it should use <h1> tag instead. You should think about when to use <h1>, <h2> and use class attribute to style instead.
- Fix: FrontendMentor, RIP English. Change
<h2>New</h2>to<h2>News</h2>. - Improvement: For font-size, you should use
:root { font-size: 16px; }for your base element, use rem for your block and use em for your element inside your block. When you do it like this, people with eye problem could resize your font-size with Ctrl +/-. - Improvement: For class attribute that being used in JS. You should probably add js- prefix to it like this navbar -> js-navbar. Because if someone ever to decide they want to work with you. They could rename class attribute value. Now, your JS doesn't work anymore :v.
- Improvement: Change
<div class="attribution"></div>to<footer class="attribution"></footer>. - Alternative: You could use
<div>01</div>instead of<span>01</span>because both of them are non-semantic tags. But <div> tag add a line break after your content.
If you have any questions or need further clarification, feel free to reach out to me.
Really well done on your part. I try to find errors. But there's not much to be found.
Happy Coding! <3 😊
0 - Taqib• 390
@taqh
Submitted
This is my first JavaScript heavy project I had a lot of problems while building especially with the form and switching sections it was also really hard to get it to display properly for various screen sizes but I did my best.
BÌnh• 110@Binh2
Posted
Hey there! 👋 Here are some suggestions to help improve your code:
Congrats on completing your challenge! 🙌
I have 10 suggestions for you:
- Add checked attribute
<div class="plan arcade" />to always check one even if the user haven't check anything. - Since this is a form page. I think you should wrap your page in <form> tag instead of <main> and <section> tags.
- You should use
<input type="tel" />instead of<input type="number" />for telephone. Because telephone can sometimes be written as "0123-456-789" (invalid number, btw) or (415) 555-2671 (can't type parenthesis in type="number") - You should use
input[type="text"], input[type="tel"] { cursor: text; }instead of cursor: pointer. - Use <img> tag with alt attribute instead of using CSS background-image when the image relate to the content. This have the added benefit of improve your SEO, improve accessibility and people who print your webpage will see your image.
- Clicking on "Monthly" and "Yearly" text in "Select plan" doesn't do anything. You could do .option { cursor: default; } to show that it's not clickable or add JS events to those.
- Your "Go back" and "Next step" buttons doesn't stay in 1 place each time I navigate
- The finish up page only being display statically. You should make the finish up page dynamically updates based on the choices that were made in previous step.
- Pick add-ons only work correctly when I clicked on text. Should also work when I click on container. I think part of the problem is you only set data-checked property to true. You should prevent the default event of checkbox and set the checked attribute in JS manually.
- Bonus: When the user click on your 1 or 2 or 3 or 4 "sidebar", your page will navigate to "Your info", "Select plan", "Add-ons" and "Summary" page respectively.
If you have any questions or need further clarification, feel free to reach out to me.
Happy Coding! <3 😊
Marked as helpful
1 - Add checked attribute
- Julieta• 60
@julietaxcc
Submitted
First try, it cost me a bit but we did it (I think soo). Do you have any tips??
Thank you in advance!!!
BÌnh• 110@Binh2
Posted
Hey there! 👋 Here are some suggestions to help improve your code:
Congrats on completing your first challenge! 🙌
I have 9 suggestions for you:
- The component is too small on larger screen. I think you should base your design on vh (view height) so the component scale correctly for other screen type.
.container { box-shadow: 10px 10px 20px #bbb; }to add shadow- You don't need CSS flexbox because using <img>, <h1>, <p> tags automatically break line.
- You can do some reset CSS styles. By resetting your styles, you avoid defaulting to the browser’s built-in styles, which differs from browser to browser. 👇
* { margin: 0; padding: 0; box-sizing: border-box; }Because on Chrome, your <h1> tag have a large margin. - You should use
img { width: 80%; }to size your image dynamically depending on parent width. - You should also use the same width for
.container2 { width: 80%; } - For improved accessibility 📈 for your content,
:root { font-size: 16px; }to give a default font-size. And, it is best practice to use rem for your container font-size and other property value. Use em for your elment. Using these units will give the users the ability to scale elements up and down, relative to a set value. - Add alt attribute to <img> tag to improve accessibility for disabled people.
- Replace <div class="container"> with the <main> tag to fix the accessibility issue.
If you have any questions or need further clarification, feel free to reach out to me.
Happy Coding! <3 😊
0 @ViniciusCassiano2105
Submitted
BÌnh• 110@Binh2
Posted
Português
Ei! 👋 Aqui estão algumas sugestões para ajudar a melhorar seu código:
Parabéns por completar seu primeiro desafio! 🙌
Tenho 6 sugestões para você:
- O componente é muito pequeno na tela maior. Acho que você deve basear seu design em vh (altura da visão) para que o componente seja dimensionado corretamente para outro tipo de tela.
- Use a tag <h1> para título em vez de <p>.
- Você não precisa fazer uma réplica perfeita da imagem do design. Acho que você deveria usar
<p>Scan the QR code to visit … </p>em vez de
<span> <p>…</p> <p>…</p> <p>…</p> </span>- h1 { font-weight: bold; font-size: 2em; } para destacar o texto do título.
- Para combinar a cor do texto na imagem de design.
h1 { color: hsl(218, 44%, 22%); } p { color: hsl(218, 44%, 22%); }- Acho que você deveria colocar um atributo alt mais descritivo como "QR code to redirect you to Frontend Mentor" porque as pessoas com deficiência têm muita dificuldade para navegar pela web. Cada pedaço ajuda.
- Substitua <div id="centralizar"> pela tag <main> para corrigir o problema de acessibilidade.
footer { text-align: center; }ficará melhor.
Se você tiver alguma dúvida ou precisar de mais esclarecimentos, sinta-se à vontade para entrar em contato comigo.
Codificação feliz! <3 😊
Portanto, a página da Web será dimensionada proporcionalmente ao tamanho da tela, em vez de depender de um formulário de largura fixa #centralizar
English
Hey there! 👋 Here are some suggestions to help improve your code:
Congrats on completing your first challenge! 🙌
I have 6 suggestions for you:
- The component is too small on larger screen. I think you should base your design on vh (view height) so the component scale correctly for other screen type.
- Use <h1> tag for title instead of <p>.
- You don't need to make a perfect replica of the design image. I think you should use
<p>Scan the QR code to visit … </p>instead of
<span> <p>…</p> <p>…</p> <p>…</p> </span>- h1 { font-weight: bold; font-size: 2em; } to make your title text stand out.
- To match the text color in design image.
h1 { color: hsl(218, 44%, 22%); } p { color: hsl(218, 44%, 22%); }- I think you should put a more descriptive alt attribute like "QR code to redirect you to Frontend Mentor" because disabled people have a really hard time to navigate around the web. Every bit helps.
- Replace <div id="centralizar"> with the <main> tag to fix the accessibility issue.
footer { text-align: center; }will look better.
If you have any questions or need further clarification, feel free to reach out to me.
Happy Coding! <3 😊
So the webpage will scale propotionately to screen size instead of relying on fixed width form #centralizar
Marked as helpful
0
