@BbeaiSubmitted over 2 years ago
This is my second challenge! Feedback welcome Thanks!
Mo'men Ashraf
@Moamen31All comments
- @Moamen31Posted over 2 years ago
Hi, good job and congrats on solving the challenge. I have some tips that could help:
- You can give the body min-height of 100vh and add align-items to it so the card will be positioned in the center of the page
- You can reduce the padding of the card and make it 10px and increase it in the text section
hope this helps and good luck keep the hard work!
Marked as helpful1 - @correlucasSubmitted over 2 years ago
š¾ Hello, Frontend Mentor coding community. This is my solution for the Article Preview Component.
How can I fix this accessibility error?
"Form elements must have labels Context: <input type="checkbox" name="share" id="share__icon">"
What I've found harder to apply on this project, was the social media popup, once I didn`t know JavaScript yet I struggled to apply the same effect using only Html + Css and I achieved the same result for the popup on the desktop version, in the mobile version I don't have any clue how to reach the same result (on/off) to show the social media icon popup and shut off the author name section when one is active and the other dont.
If you've any tip how can I achieve it entirely with Html + Css, you're welcome.
In some months I'll try this challenge again and apply the changes using Javascript.
Thank you!
@Moamen31Posted over 2 years agoHello, Lucas. congrats on finishing the challenge. I have some tips for the mobile version:
- I see that you made another div for the author for mobile, however you do not need that, you can show the popup on the author div with position absolute and z-index. And don't forget to give the parent element position relative.
- Also give the button position relative and z-index:2; to make it appear on the popup
Marked as helpful0 - @mdahsanulkabirSubmitted over 2 years ago@Moamen31Posted over 2 years ago
Hello, Mohammed. congrats on finishing the challenge. i have a few tips for you:
- try to give the body min-height of 100vh.
- for the background image you added it in the body and also in the container, it should only be in the body.
- you can also add cursor: pointer; on the icons and add transition on hover -lastly check the accessibility issues
Good luck!
0 - @PhisherFTWSubmitted over 2 years ago
This was a really good challenge, introduced me to how useful changing classnames with DOM is and how to implement some animations although I still have 2 problems I need help with.
- I haven't found a simple way to make it so everytime I open a different FAQ question the others close automatically, every solution I found uses Jquery or Bootstrap and not plain javascript, so currently you have to close each one individually otherwise if you open all of them it messes up the positioning and looks.
- On the solution is shows the corner of the left image is hidden behind the background, there might be a way to do this with overflow, but I couldn't figure it out and I tried hiding it with z-index but I had no luck because then the background would be a higher z-index than the img and hide everything.
If you have any tips or solutions I would be very happy to have input of any kind.
Thanks!
@Moamen31Posted over 2 years agoHello Keagan, well done and congrats on solving the challenge. I have some tips that might help. 1- for closing the question automatically in the JS file you have to add a loop that goes through all other questions before your function of adding the class, by using the forEach() function and then remove the class open from all other questions. 2- you can also give some transition to the rotating arrow and the head of the question maybe: 0.5s. 3-you can also add align-items to the question to have the text and arrow in the center 4-for the image on the left I believe you should move it with position relative and give the div parent of the image overflow:hidden; 5-for the background image of the main tag you can give it: background-size: cover; background-repeat: no-repeat; and you can move position it with background-position 6-you can also give the main tag width of 90 or 95vw and margin:0 auto; I hope this helps a bit and good luck.
Marked as helpful1 - @Facu3071Submitted over 2 years ago
It's my second challenge resolved, i really appreciate it any feedback.
@Moamen31Posted over 2 years agoCongratulations on finishing your challenge!
I have some feedback on this solution that might help:
- Give The body these properties to center the element
display:flex justifiy-content:center align items: center. Or center it withposition: absolute top: 50% left: 50% transform: translate(-50%, -50%) - When using article you should give it a heading h1-h6
- Check the HTML report to fix accessibility issues
Good job.
Marked as helpful1 - Give The body these properties to center the element
