Patryk Kurek• 200
@Patryk255
Submitted
Danilo Blas
@Sdann26
All comments
- Danilo Blas• 6,300
@Sdann26
Posted
Hi Patryk!
It has turned out quite well but there are some things to improve.
The texts order Summary and Annual Plan have the following color assigned hsl(223, 47%, 23%)
Also add the background color to the body background-color: hsl(225, 100%, 94%);
You could also add animations as a small transition, for example to this rule of your CSS transition: all 300ms
#card .cardFooter button { ... transition: all 300ms; }You can apply this to any rule that has hover, active, focus, etc.
Good coding!
Marked as helpful
1 - Pedro Chaparro• 260
@PChaparro
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Pedro te ha quedado bastante bien el proyecto aunque te recomendaría intentar ser más preciso con los detalles.
Darle un min-heigth a la clase quote hace que se vea mucho espaciado entre la linea que separa la frase y el botón. Te recomendaría en todo caso quitarselo y a la clase quote darle un padding-bottom de 3.5em (Uso em ya que veo más recomendado usar medida relativas que absolutas en este caso.
Entonces te quedaría así:
position: relative; text-align: center; font-weight: 800; width: 100%; max-width: 640px; border-radius: 32px; padding: 32px 42px; padding-bottom: 32px; background-color: var(--neutral-dark-grayish); padding-bottom: 3.5em;Además a la paralabra ADVICE yo le pondría el atributo letter-spacing: 4px. Este atributo permite darle separación a las letras y que quede como el diseño.
Bueno eso es lo que te recomendaría mejorar para que quede más fiel al diseño.
Buena suerte :D.
Marked as helpful
1 - PCodingTube• 510
@CodinGitHub
Submitted
Any feedback is wellcome!
Danilo Blas• 6,300@Sdann26
Posted
Hola CodingTube!
Tu maquetación te ha quedado bastante limpia, me gusta. La funcionalidad esta bastante bien. En lo personal solo le cambiaría 4 cosas:
a. Al <section class="rating-state"> le quitaría el margen, ya que no ayuda al centrado responsive ya que los margenes son estaticos, centra bien el horizontal pero en vertical no lo hace y por ejemplo en mi pantalla el componente se ve para marcar más abajo de lo que se debería, en cambio quitale los margenes y al body colocale además los siguientes atributos:
{ display: flex; flex-direction: column; justify-content: center; align-items: center; min-height: 100vh; }b. Le cambiaría el color de la letra del footer ya que no contrasta bien, no digo que deba resaltar mucho pero el negro es muy opaco más bien utilizaría un color claro por ejemplo: color: rgba(149, 158, 172, 0.5);
c. Agregale transiciones, a los botones, esto da un toque más profesional y es bastante amigable a la vista. A los elementos con las clases :hover, :active, etc. le puedes colocolar el atributo transition: transition: all 250ms;. Puedes leer la documentación para ver que valores le puedes dar.
d. Coloca todo el <section> dentro de un <main>. Por temas de accesibilidad siempre debes tener 1 por página que englobe todo el contenido principal. Por cierto, además de hacer eso debes colocar un h1 tambien por página, puede agregarlo dentro del <main> y colocarlo con la clase sr-only que tengas los siguiente atributos con la finalidad de que este presente en el esquema de tu página pero no se pueda ver para que no altere el diseño:
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0,0,0,0); border: 0; }Hay un tema más si revisas el reporte, sale que no te recomienda usar los atributos ARIA que has usado, capaz no son permitidos en el elemento que has usado, pero te recomiendo que los elimines ya que el propio reporte te lo recomienda, una ves elimines todos los errores del reporte, generá uno nuevo :D.
Y solo eso buena suerte.
0 - Alejandro• 90
@AlejandroLR00
Submitted
I have finished this project, it seemed simple to me. Could you give me some comment about my code.
He terminado este proyecto, me pareció sencillo. Me podrían dar algún comentario acerca de mi código.
Danilo Blas• 6,300@Sdann26
Posted
Hola Alejandro!
He revisado tu proyecto y respecto al maquetado te ha quedado 10 de 10, solo pequeños detalles con los tamaños pero te ha quedado en lo personal bastante bien.
Respecto a tu código, si es que usas visual studio code o algún otro te recomendaría usar la extensión prettier para que formatee tu código y se vea mucho más "bonito", ya que entre etiquetas dejas vacios innecesarios además que es más legible cuando las etiqueas padres enmarcan a las hijas. Como recomendación por mi parte te diría que estudies metodología BEM para el nombramiento de tus clases, hará que todo se más ordenado y sencillo.
Por ultimo debes eliminar los errores que te presenta el reporte de frontend mentor ya que eliminarlos te ayudará a mejorar bastante, para ellos agregan un <main> encima de <footer> y agrega todas las etiquetas dentro de <div class="card"></div> y esta dentro de <main>. Siempre debes tener un main que contenga todo el contenido principal de tu página una vez por cada página.
Y con esto listo puedes generar un nuevo reporte y no deberían salirte ningún error.
Eso seria todo, espero haberte ayudado :D!
Marked as helpful
0 - Majito• 30
@Majito1507
Submitted
¿Alguna sugerencia?
Danilo Blas• 6,300@Sdann26
Posted
Majito, felicitaciones por acabar este reto!
Te ha quedado bastante bien, ya mejorarlo solo son pulir pequeños detalles en el padding o hacer la sombra más traslucida ya sea usando en ves de la variable del color una variable propia para sombras con transparencia.
Tambien has tenido en cuenta tener 0 errores así que te ha quedado genial. Ya para mejorar a futuro puedes ir estudiando:
- Reset.css y Normalize.css, para resetear los estilos de los navegadores por defecto.
- Metodología para el nombramiento de tus clases, podría ser BEM ya que es la más sencilla y más usada.
- Aprender SASS que es superset de CSS el cual le da más cualidades que ya tiene por defecto.
Solo eso por mientras si es que no lo sabes. Good Coding! :D
Marked as helpful
2 - CARREON• 300
@byCARREON
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Congratulations on finishing this challenge, CARREON!
I would only recommend you to use some methodology for the naming of your classes as BEM but maybe you have seen it unnecessary as it is a small project. What I would mention is to change the <div class="attribution"> to <footer class="attribution"> since you are using it as this in your project, besides all the tags with content inside should be inside landmark tags (nav, main, footer, aside).
That's all I could recommend you, you've got 10 out of 10 hehe.
Good Coding!
Marked as helpful
0 - Guillermo Mulvihill• 190
@guilleoem
Submitted
What do you think? I hope you like it! Any comments are wellcome!
Danilo Blas• 6,300@Sdann26
Posted
Guillermo, felicitaciones por acabar este reto!!
Respecto al maquetado te ha quedado muy bien el diseño esta bastante fiel al diseño final. Has usado bastante bien las etiquetas aunque el texto Challenge by Frontend Mentor. Coded by Guillermo Mulvihill. le podrías dar un color un poco más claro para que haga contraste con el fondo azul marino.
Respecto al div con la clase .user podrías darle border-top: 1px solid hsla(214.9, 51.6%, 70%, 0.3), para que sea un poco más transparente como el diseño final.
Un tip para que tus diseños tengan un acabado más profesional es agregarle transiciones, osea con el atributo transition puedes generar que haya un retraso para pasar de un estado a otro generando un efecto de transición y no espontaneidad en los elementos que tienen :hover, :focus, :active, etc. Por ejemplo a la etiqueta a que tiene el nombre Jules Wyvern puedes agregarle el atributo transition: all 240ms y verás como queda con efecto bastante bueno. Por si quieres la documentación: https://www.w3schools.com/css/css3_transitions.asp
Lo único que te recomendaría cambiar en tus etiquetas es el h2 por h1 ya que siempre vas a necesitas uno por página en tu proyecto como titulo principal y a tu proyecto el falta ya con ese cambio puedes generar un nuevo reporte de frontend mentor sin errores.
Y bueno esa son las únicas recomendaciones que puedo darte por ahora, buena suerte :D!
Marked as helpful
1 - Freivys Paredes• 110
@freivys
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Freivys Paredes, felicitaciones por acabar este proyecto!
Revisandolo te recomendaría darle un poco más de tamaño a la cabezera como de 200px para que no salga tan recortada.
Por otro lado el fondo en tamaño mobile debería ser background-size: contain.
Te recomendaría agregar transiciones a los botones y enlaces de la siguiente forma transition: all 200ms (Puedes agregarle los valores que quieras). Esto le dará un acabo más profesional al pasar el cursor, o hacer click, etc. a tu proyecto.
Por cierto corrige los errores que te ha generado el reporte de frontend mentor, por ejemplo has usado src="" alt="" en un div los cuales no son atributos de este ya que son solo para imagenes así que lo mejor sería eliminarlos. Masomenos los errores que salen te dan una idea como corregirlos pero si no sabes me avisas por aquí. Apenas corrijas esos errores puede generar un nuevo reporte para que salga sin ningún error.
Por lo demás te ha quedado todo bien, buen trabajo :D!
0 - PKristin Brooks• 40
@kristinbrooks
Submitted
My first time making something like this without a tutorial. Now I just need tons more practice. 😂
Danilo Blas• 6,300@Sdann26
Posted
Congratulations on finishing your first frontend mentor project Kristin!
Whenever you finish a challenge I always recommend that you review the bug report that frontend mentor generates because it helps you fix bugs that may be more difficult to detect in the future. In your case we see that you have some accessibility errors that can be easily fixed by doing the following:
- Change
<div class="qr-component">to<main class="qr-component">. - Change
<div class="attribution">to<footer class="attribution">. - Change
<p class="text-lg">to<h1 class="text-lg">.
In the first two cases these are landmark tags that serve to encompass large sections of the project. You should always have a main with the main flow of the project while the footer is more for the bottom section of the projects that usually have one or another detail like links to social networks, the disclaimer, some navigation links, etc.
By the way you should always have a single h1 at first instance you will see that the title of the page should not be the title of the card because it works as a component that will be repeated on several occasions but if you change it will serve you so that at least in this project will not give you any accessibility problem.
And that's all I hope my comment can help you, my pleasure!
1 - Change
- sb101-beep• 60
@sb101-beep
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Congratulations on finishing your first challenge, sb101-beep!
I would recommend you mainly when working with box shadown always play with the opacity for the fading effect for example box-shadow: 1px -1px 8px -3px #1f32515c; you could also make use of functions like rgba or hsla to assign color, where the a is the alpha character that adds transparency to the color.
Finally you have to add a main tag inside the body, this is in charge of containing all the main flow of the page. You could try changing <div class="container"> to <main class="container"> to fix this.
This will generate a new report and you should get 0 errors.
I hope my comments are helpful :D!
Good Coding!
Marked as helpful
0 - Artiom Kalugin• 60
@Yiaoma
Submitted
Any feedback is appreciated.
Danilo Blas• 6,300@Sdann26
Posted
Hi Artiom Kalugin!
Your challenge is quite good, personally I think you could improve it a bit more by giving less margin to the left and right for the title and paragraph for example:
.h1 { font-size: 22px; color: rgb(31, 49, 79); margin: 24px 10px; }.p{ font-size: 15px; color: rgb(125, 136, 158); margin: 0px 10px 24px 24px; }And give the title less width.
.card { padding: 16px; border-radius: 20px; background-color: rgb(255, 255, 255); text-align: center; display: flex; flex-direction: column; width: 280px; }And I would reduce the border-radius to 12px of the QR image.
Finally you have two accessibility errors and both are generated by not placing the main tag inside your body.
To do this just change <div id="app"> to <main id="app"> or place a main tag inside it. The main reason is that semantically you should always have one that encompasses all the main content of the page.
I hope my comments are helpful :D!
Good Coding!
Marked as helpful
1 - Iqra Zaheer• 20
@iqra0001
Submitted
Any kind of feedback would be appreciated.
Danilo Blas• 6,300@Sdann26
Posted
Hi iqra0001!
Regarding the design you have done well but try to remove the comments from your HTML as it is creating conflicts in the frontend mentor report.
Also change the <div class="main-container"> to <main class="main-container"> as it is semantically more accurate to use it in this context and in the end it has the functionality of encompassing all the main content of your project, being necessary to always put only one inside the body.
Don't forget to generate a new report after correcting so that the challenge comes out without any error.
If you can make the card slightly larger so that the word to goes on the second line.
With those details it would be perfect.
Hehehe good coding!
Marked as helpful
1 - Emmanuel• 20
@Horezzy2022
Submitted
Please kindly go through my codes and help if there was any error met or unneeded styles. Thank you!!!
Danilo Blas• 6,300@Sdann26
Posted
Congratulations on finishing this challenge.
It's not that you have a lot of coding errors rather there are some details that you have not taken into account.
For example if you look at the frontend mentor report you may notice that you need to use landmark tags that encompass large sections of code including the <main> which is a necessary tag in your code to contain all the main content of your page. Then make the following changes.
Change:
- <div class="container"> by <main class="container">
- <div class="attribution"> to <footer class="attribution">
This will solve some problems in your report.
Finally you always need to have one h1 and only one in your HTML schema and these h... should be in increments so you better edit the ones you have and use better modifying tags:
- <h3 class="main-text"> by <h1 class="main-text">
- <h5 class="text"> by <p class="text">
With these changes you shouldn't have any errors so try to use better HTML tags to avoid these problems, good luck :D!
0 - TamVik• 30
@TamVik
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Hi TamVik!
Your challenge looks really good, I would only recommend you to center it vertically as follows:
body { display: flex; justify-content: center; align-items: center; min-height: 100vh; }At most I will tell you to add a little bit of shadow to the card to make it look like the design but you have done quite well, congratulations!
Marked as helpful
0 - Pratyusha Majumder• 280
@FloraBloomblue
Submitted
This was hard for me. I tried. However, I have got a few questions. It would be really kind of you guys if you help me with it.
-
The image in black and white and I have to use a color over it. so the image with black and white has shade of color on it. I saw the overlay method, linear gradient. . Finally I applied it but I think there is something wrong with my code. Mine is not coming similar to the original design.
-
I have done the overlaying thing, but when I am increasing the resolution, that overlay div changes position , so it's not fixed over the image.
-
Now coming back to the color thing, given is hsl format, linear gradient works better with rgba. So, I thought of converting hsl to rgb, but I was not satisfied, so kept hsl, and used opacity with it. But I still couldn't come close to the given picture in the design.
That's all , Thank you
Danilo Blas• 6,300@Sdann26
Posted
Hi Pratyusha!
This challenge is a bit annoying, ideally you should concentrate on doing the mobile design first and then the desktop design or vice versa. At the end you do what you are asked to do, the trick is to apply a parent div and inside the img image as its child. To the father apply a background-color that corresponds to him that I suppose that it is like pink and to the son give him mix-blend-mode: multiply and something of opacity and it will be equal to the design.
The idea is that mix-blend-mode allows to merge an image with the color of the parent in this case the div that I mention to you.
If you want to see how I applied it you can check the challenge when I did it so I also made it responsive.
Don't worry, the experience comes little by little.
Good Luck
Marked as helpful
2 -
- archis• 260
@archisvaze
Submitted
Learned a lot about Hover states in CSS and using flexbox!
Danilo Blas• 6,300@Sdann26
Posted
Archis with respect to the design and effects you have been perfect.
I will give you recommendations on how to improve your presentation in a more professional way.
For example, to the links and buttons you can add the attribute transition: all 400ms; this will make them go from their natural state to :hover, :active, :focus, etc. I realize it by means of a transition giving a smoothing that is quite aesthetic. Use it for the NFT name, the user and the overlay.
On the subject of accessibility I think you already corrected the errors but if or if you change <h4 class="title">Equilibrium #3429</h4> by <h1 class="title">Equilibrium #3429</h1> since you should always have at least one per project.
By the way, do not try to use only <div> for text there are tags like <span>, <strong>, <p>, <em> tags that serve this purpose as well.
Try those changes and generate a new report to see if all the errors are eliminated.
I hope my comments are helpful.
Marked as helpful
1 - Irina• 80
@irinani
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Irina, hi!
This challenge has also been pretty good, if you want to eliminate that problem by not having placed a h1 you can do it by placing inside the <main> a <h1> with the text you want inside and place the class sr-only.
This class has the following attributes:
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0,0,0,0,0); border: 0; }Its main function is not the one we are placing, but it will do for now. If you want to know more about it, search in web accessibility but you can also learn it in the responsive HTML and CSS course of freeCodeCamp.
And with this you can update the frontend mentor report and not get any errors.
Good Coding!
0 - Irina• 80
@irinani
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Good work! You have been perfect, congratulations!
0 - Yokke• 640
@Jexinte
Submitted
Hello All ,
Could I get some feedback ?
I wasn't able to deal with one of the background on the desktop mode and couldn't manage the behavior of the women on desktop when a question is hide / show . I don't know if it is normal.
I struggle with the box and women on desktop the overflow method haven't work like I would so I got some help with google :)
It not fully responsive under 600px.
In hope that someone can help me !
Thanks for reading !
Danilo Blas• 6,300@Sdann26
Posted
Hi Yokke!
To be honest I don't think my comment will help much because personally I would change a lot of things but I'll tell you the picture I have.
On the one hand I would recommend you to use the <summary> and <details> tags, these by default have the toggle effect that you are looking for in this challenge and with JS you can control that when you click on one the others are closed and thus in addition to remove and give the classes for when a question has been selected.
If you want you could see the solution I made for this challenge using those tags, also pseudo elements for the arrow and the CSS effects to make it rotate as well as the text appear and fade.
On the other hand I see no need for the <article>. Remember that <section> and <article> are tags to encompass sections of the web but are sections with the same theme does not make sense to use them in a component as they all follow the same theme of questions so I would recommend not to abuse these classes also then you will jump accessibility errors like this in your report.
The intention has been quite good but there are things that you can improve to make it perfect. I hope my comment can serve you even a little bit as a guide.
0 - Zuliatu• 140
@zuliat
Submitted
All feedback would be appreciated. Thank you.
Danilo Blas• 6,300@Sdann26
Posted
Hi Zuliatu!
I really like your finished project because it has responsive, quite well done, plus transitions. It's pretty good.
Just fix the report problem which is quickly fixed if you change <div class="attribution"> to <footer class="attribution">.
Lastly, I would recommend you not to use too many h1 because it penalizes the positioning of the page. There should only be one per page so it is best to change them all to h2. The problem is that then you should add an h1 for accessibility issues as there should always be one per page so inside the main place one and give it the class sr-only with this it will disappear from the visible flow but it will still be present on the web.
This class has the following attributes:
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0,0,0,0,0); border: 0; }Its main function is not the one we are placing, but it will do for now. If you want to find out more about this search on web accessibility but you can also learn it in freeCodeCamp's responsive HTML and CSS course.
And if you have any questions, feel free to ask me here.
Good Coding!
Marked as helpful
1 - Martin• 220
@martinw0
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Hi Martin!
If you want to avoid that blue color when you click on the input you can set outline: none.
.form input { border: solid hsl(0deg, 36%, 70%) 1px; border-radius: 25px; height: 44px; width: 100%; box-sizing: border-box; background: transparent; padding-left: 20px; outline: none; color: hsl(0, 6%, 24%); }Although you can also change the color if you wish to your liking. Also by the way I added the brown color since the text if I'm not mistaken is brown not black.
Also for the image of the desktop version I would add a background-size like:
.img { height: 31vh; background: url("./images/hero-mobile.jpg"); background-size: cover; }Finally as a personal experience I think you could do validations to see if it is an email using RegeX, it is a little messy to create it from scratch but on the internet there is already RegeX to verify if you are inserting an email. That would add a layer of security which is always good to add through frontend :D!
If you have any questions feel free to ask me.
Marked as helpful
0 - fikri• 30
@Recker27
Submitted
Danilo Blas• 6,300@Sdann26
Posted
By the way I forgot, frontend mentor gives you 5 screenshots to preview the final result of your challenge, don't forget that after you change the design and you see that you like it, generate a new screenshot so you can see the last changes :D!
Just that, good luck!
0 - fikri• 30
@Recker27
Submitted
Danilo Blas• 6,300@Sdann26
Posted
Hi fikri!
Your challenge you have done well but I will give you some personal recommendations that I think you should keep in mind.
The first thing is the theme is too thin if you can make it wider to the card like with width: 360px;.
Also if you want to make the background look better, change the background-size attribute.
body { background-image: url(images/pattern-background-desktop.svg); background-size: contain; background-repeat: no-repeat; background-color: hsl(225, 100%, 94%); font-family: "Red Hat Display", sans-serif; }
Placing contain works and looks like the design.
Finally there is a detail that I think you have overlooked but you have not put the active states. If you can put them. You can see this in the design folder and inside there is an image that should say active-state where you can see how the component would look like when you hover the cursor over the buttons or links.
And with that I think I could improve the final design of the challenge :D!
If you have any questions you can ask me here.
0 - Khairul Fadhli• 160
@kfadhz
Submitted
Hi, this my solution for this time. I know it's not 100% same, but comments are welcome to make it better. I use Atom to build this site. For some reason, I don't know why background image not appear, I think I wrote the right code. I intend to learn more and more to make it better and always learning, thank you...
Danilo Blas• 6,300@Sdann26
Posted
Hi Khairul!
The reason why you can't see it is because your project is also in a folder, the po repository does work as one, so your link is the following:
https://kfadhz.github.io/Clipboard-Landing-Page-Challenge/Clipboard%20Landing%20Page/I was looking at your project and personally to the download buttons for windows and Mac I would give more padding in general.
I would also add effect to the links that appear in the footer as they are usually links to navigation as well as to the social networks so that they work as links that take you outside.
Also I recommend that you see the report that has generated many errors in if most is because you have not used the <main> tag you have used by <section>. I would recommend you to put all the <section> inside a <main> except the last one that you could change it to a <footer> tag. Try to use more semantic tags as that improves your page ranking when uploaded.
For now that's what I can recommend you try to make those changes especially the tags until your report is error free, it will make you a better front-end developer. Good luck!
0
