Please any suggestions are welcome.
Freddy Santana
@CreixzAll comments
- @aheduardo5Submitted 9 months ago@CreixzPosted 9 months ago
Hello my friend, congratulations for finish this challenge, i like your BEM methodology and the mobile-first design, but i have some suggestions for you:
- It's better to use rem instead of pixels.
- The color of the
<p>
and<li>
are incorrect. - Use pseudo-elements like
::before
, it could help you with yourlist-style
Marked as helpful0 - @BenheminSubmitted 9 months agoWhat are you most proud of, and what would you do differently next time?
.
What challenges did you encounter, and how did you overcome them?.
What specific areas of your project would you like help with?.
@CreixzPosted 9 months agoHello my friend, congratulations for finish this challenge, let me show you some details that can help you to improve your code.
- It's better to use good classnames that can make your code more understandable, I use BEM like this:
<section classname= 'card'> <article classname='card__title'> <p class='card__title-description'>Your text here.</p> </article> </section>
- Use
margin-left=auto
andmargin-right=auto
to center your card. - It's better to use rem instead of % or pixels.
- Use max-width instead of width, it will look better.
0 - @luizariloSubmitted over 2 years ago@CreixzPosted over 2 years ago
Hello my friend, Congratulations for finish the project, its seems pretty good, but I have some suggestions regarding your solution if you don't mind:
- Use BEM methodology it's a good practice, like this:
<div class="card"> <div class="card__header"> <img class="card__img" src="images/image-qr-code.png" alt="Qr-Code">
- Put the card inside the container, like this:
<section class="container"> <div class="card">
- Don't use and specific
height
for the img, just usewidth= 100%;
to adjust to the container and not use unnecessarymargin-top
.
Thank you for taking the time to read my feedback I hope it helps.
Marked as helpful0 - @ofekshmuelySubmitted over 2 years ago@CreixzPosted over 2 years ago
Hello my friend, Congratulations for finish the challenge, its seems pretty good, but I have some suggestions regarding your solution if you don't mind:
- You can implement BEM's methodoly because it's a good practice and its not so difficult. Example: Instead of
<div class="container"> <div dclass="container"> <img class="qrcode" src="./images/image-qr-code.png" alt="">
use this
<div class="container"> <div class="card"> <img class="qrcode" src="./images/image-qr-code.png" alt="qrcode" class: card__image>
- Use
max-width: 350px;
in yourcard
because its a mobile design and then usewidth: 100%
in yourcard__image
to adjust theimage
to thecard
Thank you for taking the time to read my feedback I hope it helps.
Marked as helpful0 - @MURRAY122Submitted over 2 years ago
For the image overlay. Is there a way to change or control the fill color of a background image SVG so for example:
background-image: url("link to some svg file");
When I used this and lowered the opacity of the parent element, the white color of the SVG eye image also lowered in opacity. So, I tried to research and find if the SVG's fill color could be changed while being used as a background-image and found nothing. So, I would have to assume there isn't a way? In the end I ended up using a second div and image tag to hide and show when hovered on.
Any other feedback is always appreciated, thanks for viewing
@CreixzPosted over 2 years agoHi Murray122, first of all contratulations for complete the challenge, I think you do a pretty well job and I have some suggestions regarding your solution if you don't mind:
Instead of put the
icon-view
in your html, I think its better to put in your css code, like this:.card .image::before { content: url(images/icon-view.svg); position: absolute;
I saw that you use BEM's methodology, but i think that instead of this:
<div class="section__content"> <h1 class="section__content__title"><a href="#">Equilibrium #3429</a> </h1>
you can use this:
<div class="section__content"> <h1 class="section__title"><a href="#">Equilibrium #3429</a></h1>
because it's not necessary to repeat all the name, i saw in many post that is not a good practice. Hopefully this feedback helps.
0