Ahmed Bayoumi
@Bayoumi-dev
All comments
- Ahmed Bayoumi• 6,800
@Bayoumi-dev
Posted
Hey Tadem, Congratulations on completing this challenge... You have
accessibility issues
that need to fix.Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
-
I suggest you use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS? -
Also, I suggest you look into an approach called
"BEM"
Block Element Modifier
Hope this help!... Keep coding👍
0 - sujeong• 40
@sujeong054
Submitted
how to do an active design? I use the hover tag to change the color of the text. but I can't solve the img tag change.
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Sujeong, It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="card"> //... </div> </main>
- Use
<a>
to wrapEquilibrium #3429
andJules Wyvern
, then addcursor: pointer;
to the<a>
, The cursor indicates to users there is an action that will be executed when clicking on it.
Equilibrium #3429
---> Navigate the user to another page to see the NFTJules Wyvern
---> Navigate the user to another page to see the creator's portfolio- To do the active state for the image on the NFT preview card I suggest you wrap the image of the NFT with the
img-wrapper
element and create another element to make itoverlay
on the image, it contains theview icon
.
<div class="img-wrapper"> <img class="card-img" src="images/image-equilibrium.jpg" alt="Image Equilibrium"> <div class="overlay"><img src="images/icon-view.svg" alt=""></div> </div>
Then add the following styles:
.img-wrapper { width: ...; /* it's up to you */ height: ...; /* it's up to you */ position: relative; /*To flow the child element on it*/ cursor: pointer; overflow: hidden; } .img-wrapper .card-img{ width: 100%; height: 100%; } .overlay { background-color: hsla(178, 100%, 50%, 0.5); display: flex; /* Center the view icon with flexbox*/ align-items: center; justify-content: center; position: absolute; top: 0; width: 100%; height: 100%; opacity: 0; } .img-wrapper:hover .overlay { opacity: 1; }
There is another way to do the active state for the image on the
NFT preview card
by creatingpseudo-elements
(::after
,::before
) to use one as anoverlay
and other forview-icon
.Resources
- How to Center Anything with CSS - Align a Div, Text, and More
- 11 Ways to Center Div or Text in Div in CSS
- Pseudo-elements
- ::after (:after)
- ::before (:before)
Hope this is helpful to you... Keep coding👍
Marked as helpful
0 - Jake Clis• 70
@clispy1
Submitted
Relaxing project to make
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! It looks good!... You have
accessibility issues
that need to fix.Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
Buttons must have the discernible text
, So don't leave thealt
attribute empty when using the image<img>
inside the button<button>
without text.
Or set the attribute<button class="change"> <img src="./images/icon-dice.svg" alt="Advice Generator"> </button>
aria-label
to describe the button.
Hope this help!... Keep coding👍
Marked as helpful
0 - Shehab Sha'lan• 30
@shehabshalan
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Shehab, It looks good!... You have
accessibility issues
that need to fix.- Documents must have <title> element to aid in navigation
<head> //... <title>3-column preview card component | Frontend Mentor</title> </head>
- Using more than one
<h1>
is allowed by the HTML specification, but is not considered a best practice. Using only one<h1>
is beneficial for screenreader users.
---> Multiple
<h1>
elements on one pageI hope this is helpful to you... Keep coding👍
1 ---This is an updated version to create a better layout experience using the suggestions from my peers.
First challenge on Frontend Mentor. Feedback welcome as I know there are multiple ways this project could have been done and probably much better.
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey John, It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main class="container"> <div class="qr_code_card"> //... </div> </main>
-
Page should contain a level-one heading
, Change<p class="p1">
to<h1 class="heading">
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
You have used
grid
to center the component on the page, but you need to set the height of thecontainer
--->min-height: 100vh;
to center it at this height:
.container { display: grid; justify-content: center; align-items: center; /* width: 1440px; /* <---- Remove */ /* height: 800px; /* <---- Remove */ min-height: 100vh; /* <--- Add */ }
- Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
Hope this help!... Keep coding👍
Marked as helpful
0- Brian Schooler• 440
@superschooler
Submitted
Any feedback is welcome! The thing I struggled with most on this project is rounding the corners of Bootstrap's .container element without Sass. I ended up having to manually round each corner and create media queries to change it based on the screen size. Any ideas how to do it on the container itself?
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Brian, It looks great!... Here are some suggestions:
- Instead round each corner and create media queries to change it based on the screen size, Give the parent these classes
rounded
,overflow-hidden
<main class="container rounded overflow-hidden" id="container"> //... </main>
- Using more than one
<h1>
is allowed by the HTML specification, but is not considered a best practice. Using only one<h1>
is beneficial for screenreader users.
---> Multiple
<h1>
elements on one pageHope this help!... Keep coding👍
Marked as helpful
1 - Instead round each corner and create media queries to change it based on the screen size, Give the parent these classes
- Jack Shelton• 210
@thejackshelton
Submitted
What could I have done to make this shorter?
I think 180 lines of CSS for this is probably a lot longer than it should be. Would love to see other solutions
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Jack, It looks good!... You have
accessibility issues
that need to fix.Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
-
I suggest you align items by using
margin
andpadding
instead ofposition
--->Alignment, margin, padding -
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
Hope this help!... Keep coding👍
Marked as helpful
0 - Etinhandy• 30
@Etinhandy
Submitted
Suggest me areas to improve on
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Etinhandy, Congratulations on completing this challenge... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main id="page-holder> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document...<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
I suggest you put
the socials
into thelist item
to add moresemantics
to your project...
<ul class="socials"> <li class="icons"><a href="https://www.facebook.com/"><img src="./images/facebook.svg" alt="facebook"></a></li> <li class="icons"><a href="https://www.twitter.com/"><img src="./images/twitter.svg" alt="twitter"></a></li> <li class="icons"><a href="https://www.instagram.com/"><img src="./images/instagram.svg" alt="instagram"></a></li> </ul>
Hope this help!... Keep coding👍
Marked as helpful
0 - Dvs Chandrasekhar• 110
@Chandra30sekhar
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
I hope this is useful to you... Keep coding👍
Marked as helpful
1 - TY• 60
@sammingty
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! Congratulations on completing this challenge... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container ..."> //... </div> </main>
-
Page should contain a level-one heading
, Change<span class="name">Victor Crest</span>
to<h1 class="name">Victor Crest</h1>
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS? -
I suggest you put the status of the profile card into the
list item
to add moresemantics
to your project...
<ul class="stats"> <li><span class="stats-num">80K</span>Followers</li> <li><span class="stats-num">803K</span>Likes</li> <li><span class="stats-num"> 1.4K</span>Photos</li> </ul>
Hope this help!... Keep coding👍
Marked as helpful
0 - Sushant nande• 30
@sushant2313
Submitted
This is my first ever challenge, so it was quite challenging. But I did it. I had trouble in the beginning on how to align the items the way I wanted. but as I googled couple of things I understood.
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Sushant, Congratulations on completing this challenge... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="grid"> //... </div> </main>
-
An
img
element must have analt
attribute, to provide alternative information for an image if a user for some reason cannot view it. -
All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
-
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS? -
I suggest you center the component on the page with
Flexbox
, by giving the parent element<main>
the following properties:
body { background-color: hsl(30, 38%, 92%); /* margin: 150px; /* <---- Remove */ //... } main { /* <--- Add */ display: flex; justify-content: center; align-items: center; min-height: 100vh; } .grid { background-color: #fff; /* margin: 0 auto; /* <---- Remove */ //... }
Hope this help!... Keep coding👍
Marked as helpful
1 - Ashutosh Bisoyi• 20
@ashutoshbisoyi
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Ashutosh, It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container flex-center"> //... </div> </main>
-
Add
cursor: pointer;
to thebutton
, The cursor indicates to users there is an action that will be executed when clicking on it. -
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
I hope this is helpful to you... Keep coding👍
Marked as helpful
0 - Hugo• 430
@HugoMoncada
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Hugo, It looks good!... Here are some suggestions:
- Use
<button class="dice-div">
instead of<div class="dice-div">
... Buttons are used for actions.
<button class="dice-div"> <img class="dice" src="/images/icon-dice.svg" alt="Advice Generator"> </button>
-
Use the
quotation
element instead of theheading
element to add more semantics to your project... ---><q>: The Inline Quotation element
-
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
Hope this help!... Keep coding👍
1 - Use
- End Dev• 150
@End-Dev-web
Submitted
Hi, Frontend Mentor community! Here is my solution for QR code component, any suggestions are very welcome
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! Congratulations on completing this challenge... You have
accessibility issues
that need to fix.Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
Page should contain a level-one heading
, Changeh2
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page.- Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
Hope this help!... Keep coding👍
Marked as helpful
1 - Gabriel Barros• 20
@gabrielbarrosg
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Gabriel, It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div id="advice"> //... </div> </main>
- An
img
element must have analt
attribute, to provide alternative information for an image if a user for some reason cannot view it, but in this case, add an emptyalt
attribute to avoid the accessibility issue because it is an unnecessary icon for the content.
<img class="divider" src="assets/images/pattern-divider-desktop.svg" alt="">
- use
<button onclick='generateAdvice()' id="advice-btn">
instead of<div onclick='generateAdvice()' id="advice-btn">
... Buttons are used for actions
<button onclick='generateAdvice()' id="advice-btn"> <img class="btn" src="assets/images/icon-dice.svg" alt="Advice Generator"> </button >
-
Page should contain a level-one heading
, Changeh3
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
Hope this help!... Keep coding👍
Marked as helpful
1 - Devendra Dantal• 110
@devendra-dantal04
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! Congratulations on completing this challenge... You have an
accessibility issue
that needs to fix.Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
Hope this help!... Keep coding👍
Marked as helpful
0 - Tomas• 50
@JoseTomas-GP95
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Tomas,
My suggestions:
- Using more than one
<h1>
is allowed by the HTML specification, but is not considered a best practice. Using only one<h1>
is beneficial for screenreader users. --- Multiple<h1>
elements on one page - Add
cursor: pointer;
to thebutton
, The cursor indicates to users there is an action that will be executed when clicking on it.
I hope this is helpful to you... Keep coding👍
Marked as helpful
0 - Using more than one
- Laura Alvarez• 80
@lalvarezz
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Laura, I suggest you put the status of the profile card into the
list item
to add moresemantics
to your project,Div
's don't do much for semantics but a list is much more meaningful..:<ul class="stats"> <li><span class="stats-num">80K</span>Followers</li> <li><span class="stats-num">803K</span>Likes</li> <li><span class="stats-num"> 1.4K</span>Photos</li> </ul>
I hope this is useful to you... Keep coding👍
Marked as helpful
0 - PEileen dangelo• 1,600
@Eileenpk
Submitted
I had to read up on mix-blend-mode to get the layering of the picture correct, but it wasn't complicated, the background-color had to be on the parent container, and the background-image + mix-blend-mode property on the child element. Any feedback is always welcome!
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Eileen, I suggest you put the status of the preview card component into the
list item
to add moresemantics
to your project,Div
's don't do much for semantics but a list is much more meaningful..:<ul class="stats-preview__stats"> <li> <span class="stat-number">10K+</span> <span class="stat-name">Companies</span> </li> <li> <span class="stat-number">314</span> <span class="stat-name">Templates</span> </li> <li> <span class="stat-number">12M+</span> <span class="stat-name">Queries</span> </li> </ul>
I hope this is useful to you... Keep coding👍
Marked as helpful
1 - BanditDev• 220
@banditdev013
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! It looks good!... Here are some suggestions:
Page should contain a level-one heading
, Changeh3
toh1
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page.- I suggest you put the status of the profile card into the
list item
to add moresemantics
to your project,Div
's don't do much for semantics but a list is much more meaningful..:
<ul class="stats"> <li><span class="stats-num">80K</span>Followers</li> <li><span class="stats-num">803K</span>Likes</li> <li><span class="stats-num"> 1.4K</span>Photos</li> </ul>
- Use
REM
for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?
I hope this is helpful to you... Keep coding👍
Marked as helpful
1 - Ramsès Jr• 30
@ramzy05
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Ramsès, Congratulations on completing this challenge... You have
accessibility issues
that need to fix.Document should have one main landmark
, Contain the main content with<main>
.
<main> <section id="hero"> //... </section> <section id="services"> //... </section> <section id="articles"> //... </section> </main>
Heading levels should only increase by one
, Useh4
instead ofh5
... Always Make sure you are using the headings in order,Ordered headings
make it easier to navigate and understand when using assistive technologies.
Hope this help!... Keep coding👍
Marked as helpful
0 - Sharon• 170
@siafromspace
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey Sharon, It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
-
Page should contain a level-one heading
, Change<p id="advice-no"></p>
to<h1 id="advice-no"></h1>
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page. -
use
<button class="dice">
instead of<div class="dice">
... Buttons are used for actions.
<button class="dice"> <img src="images/icon-dice.svg" alt="Advice Generator" class="click-dice roll-dice"> </button>
Hope this help!... Keep coding👍
Marked as helpful
1 - Shank0071• 190
@Shank0071
Submitted
Ahmed Bayoumi• 6,800@Bayoumi-dev
Posted
Hey! It looks good!... Here are some suggestions:
Document should have one main landmark
, Contain the component with<main>
.
<main> <div class="container"> //... </div> </main>
All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
- I suggest you center the component on the page with
Flexbox
, by giving the parent element<main>
the following properties:
body { font-family: Arial, Helvetica, sans-serif; /* width: 80%; /* <---- Remove */ /* margin: 40px auto; /* <---- Remove */ background-color: hsl(216, 12%, 8%); } main { /* <---- Add */ display: flex; justify-content: center; align-items: center; min-height: 100vh; } .container { //... /* margin: 0 auto; /* <---- Remove */ }
I hope this is helpful to you... Keep coding👍
Marked as helpful
1 - Ahmed Bayoumi• 6,800
@Bayoumi-dev
Posted
Hey Vanessa, It looks good!... You have
accessibility issues
that need to fix.Page should contain a level-one heading
, Change<div class="card-title">Equilibrium #3429</div>
to<h1 class="card-title">Equilibrium #3429</h1>
You should always have oneh1
per page of the document... in this challenge, you will useh1
just to avoid theaccessibility issue
that appears in the challenge report... but don't useh1
on small components<h1>
should represent the main heading for the whole page, and for the best practice use only one<h1>
per page.All page content should be contained by landmarks
, Contain the attribution with<footer>
.
<footer> <div class="attribution"> //... </div> </footer>
Hope this help!... Keep it up👍
Marked as helpful
0