@elaineleung
Macdeesh
@macdeeshAll comments
- @macdeesh
Hi Elaine, this looks really nice ! Very good work !! Bravo !!
In case the amount of the bill is a big number like more than 7 zeros, the result of the calculation in Tip amount/person and total/person is too long and take place over the calculator, to fix this add an overflow-x: scroll on the amount fields.
You forget to make the error message when the number of people is not added or 0. Or maybe it's by choice ?
Marked as helpful - @KKS1991@macdeesh
Hello Kevin, For the header-image, it looks oversized. Try max-width: 100%.
Marked as helpful - @Briuwu@macdeesh
Helloo Briuwu,
Your work looks pretty good !! Well done !! I had a look at your code just to answer to your question about the HTML issues. I'm sure that you know that the
idis unique, and we can't use more than oneidname's in our HTML. Maybe you couldn't localize the error because the project is bootstrapped with Create React App. The linear gradient is used with the Instagram SVG 3 times with the sameid, so you need to change or delete theidof 2 of those 3 SVGs. How I don't know but I found one of theidhere :<div class="social-each Light Instagram"> <div class="social-title"> <svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"> <defs> <linearGradient id="a" x1="100%" x2="0%" y1="0%" y2="100%">I think fixing it is a bit tricky, as I don't have enough knowledge in JS, I can't help you more, but if you want to fix the issue have a look at this repo : https://github.com/exogen/babel-plugin-inline-react-svg/pull/1 Or this one : https://github.com/airbnb/babel-plugin-inline-react-svg/issues/57
Marked as helpful - @obaidash99@macdeesh
Hello Obaida, That looks pretty good ! Well done ! Have a look at the report above, it shows a few issues that you can fix. I also have some suggestions for you :
-
I would recommend using pseudo-element
::beforeand::afterto put the Custom shape dividers inside the CSS. That way, you can remove the decorative elements from the HTML structure. -
Add obvious focus visible styles to every interactive element. This should be obvious and differ from the hover style, it helps keyboard users know where they are on the page.
-
For the email input, it should be
<input type="email" id="email" name="email">, and before that you should add a label :<label for="email">. -
Don't forget to write a good alt text to your image. Try to not use words like image or hyphenated phrases as if it's code, it is a human-readable description.
Marked as helpful -
- @macdeesh@macdeesh
Hey Obaida, Thank you for your advice, I already saw it in the report as an accessibility issue and to be honest it was intentional to use h2 instead of h1, because normally this card component should exist as a div inside a body of a full web project, and it has no meaning to use a level-one heading in a profile card. Anyway, thank you again, and I hope we keep in touch with constructive feedbacks for our next projects. Happy coding.
- @lilKriT@macdeesh
Hello lilKriT, Just to answer to your question, maybe you can add ".." before the URL of the background image :
background-image: url("../img/icon-supervisor.svg");Instead ofbackground-image: url("/img/icon-supervisor.svg");In your place, I will also use the pseudo-element ::after with the specific class and add :
content: url("../img/icon-supervisor.svg");Marked as helpful - @bvrkl@macdeesh
Hello Liam Burke,
This is how you can improve your code :
1- You should wrap the Equilibrium image with an anchor tag. It has :hover state, which means that it should be an interactive element. It also applies the other elements that have :hover state.
2- I would do the hover effect on this using pseudo-elements like ::before or ::after. Also, for the other SVG icons you can put it inside pseudo-elements, as the icons are all decorative.
3- For the hover effect (now attached to an interactive element wrapping the image and the eye icon) remove width on the img and set it to be display block max width 100%.
4- I would recommend using h1 instead of h2, your page should contain a level-one heading.
5- I don't recommend that you style the creator name using strong in your HTML, try font-weight inside CSS if you want to make it bold.
- @brewaskew@macdeesh
Hello Matt,
This is how you can improve your code :
1- You didn't use the icon-view SVG when hovering the image. I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML. Also, for the other SVG icons you can use pseudo-elements like ::before or ::after, as the icons are all decorative.
2- The anchor tag for the heading should be inside a heading element.
3- You should have no text in spans alone. Always use meaningful elements, like paragraphs, headings, lists etc.
4- Don't forget to write a good "alt" text to your image. Try to not use words like image or hyphenated phrases as if it's code, it is a human-readable description.
5- Font size should never be in px, always rem (or rarely em when you want it to inherit from a parent)
6- You can add a class to the anchor of the creator name and style it instead of styling the span.
Marked as helpful - @safXcode@macdeesh
Hello Safwan,
You need to make some change in the HTML:
1- I would recommend using h1 instead of h2, your page should contain a level-one heading.
2 - Your Article needs heading. Consider using h2-h6 elements to add identifying headings to all articles. Or I suggest using <div> instead of <article>.
3- Think about when you use the Web. What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. So you should wrap the Equilibrium image and the other elements with an anchor tag.
4- I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other SVG icons you can put it inside pseudo-elements like ::before or ::after, as the icons are all decorative.
5- Font size should never be in px, always rem (or rarely em when you want it to inherit from a parent)
6- Don't forget to write a good "alt" text to your image. Try to not use words like image or hyphenated phrases as if it's code, it is a human-readable description.
Marked as helpful - @Nezo96@macdeesh
Hello Rado,
This is how you can improve your code :
1- What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. That is essential to fix.
2- I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other SVG icons you can use pseudo-elements like ::before or ::after, as the icons are all decorative.
3- You should have no text in spans alone. Always use meaningful elements, like paragraphs, headings, lists etc.
4- Font size should never be in px, always rem (or rarely em when you want it to inherit from a parent)
Marked as helpful - @codderhmar@macdeesh
Hello Emanuel Hmar,
You need to make some change in the HTML:
1- Your <head> element is missing a required instance, so you must add a <title>.
2- I would recommend using h1 instead of h2, your page should contain a level-one heading.
3- Think about when you use the Web. What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. So you should wrap the Equilibrium image and the other elements with an anchor tag. That is essential to fix.
4- I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other SVG icons you can put it inside pseudo-elements like ::before or ::after, as the icons are all decorative.
5- Font size should never be in px, always rem (or rarely em when you want it to inherit from a parent)
- @Dan-Kant@macdeesh
Hello Dan Kant,
You need to make some change in the HTML:
1- Your <head> element is missing a required instance, so you must add a <title>.
2- I would recommend using h1 instead of h2, your page should contain a level-one heading.
3- Your image-equilibrium must be in your HTML because it's not a decorative element.
4- You don't have to change the format of your SVG. Use it inside a pseudo-elements like ::before or ::after to create the overlay, example :
.image-class::before { content: ""; background: url("../icon-view.svg") center no-repeat; background-color: hsla(178, 100%, 50%, 0.5); border-radius: 5px; position: absolute; width: 100%; height: 100%; opacity: 0; transition: .5s ease; top: 50%; left: 50%; transform: translate(-50%, -50%); z-index: 1; }Then use :
.image-class:hover::before { opacity: 1; }Also, for the other SVG icons you can use pseudo-elements, as the icons are all decorative.
5- Think about when you use the Web. What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. So you should wrap the Equilibrium image and the other elements with an anchor tag. That is essential to fix.
6- Don't forget to write a good alt text to your image. Try to not use words like image or hyphenated phrases as if it's code, it is a human-readable description.
- @Darko96@macdeesh
Hello Darko,
That looks pretty good, just more few things to improve your code:
1- Wrap the h1 and the author name with an anchor tag. It has a hover state, so you need to include an interactive element. That is essential to fix.
2- Also, I would recommend using pseudo-element and background properties to put the other SVG icons inside your CSS, like you did for icon view. That way, you can remove the decorative elements from the HTML structure, but that's not essential.
Marked as helpful - @Archerking47@macdeesh
Hello Archerking47,
That looks pretty good except for a few things:
1- Move the styling of attribution to your CSS file.
2- What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. That is essential to fix. So you should wrap the Equilibrium image and the other elements with an anchor tag.
3- I would do the hover effect using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other icons you can use pseudo-elements like ::before or ::after, to put the SVGs inside your CSS as the icons are all decorative.
Marked as helpful - @bodashideung@macdeesh
Hello Muhamad, that looks pretty good. I really liked the box-shadow and the transition that you used. One more thing that you can improve, wrap the Equilibrium or the NFT image with anchor tag. It has :hover state, which means that it should be an interactive element. It also applies the other elements that have :hover state. I would recommend using pseudo-element to create the overlay for the view icon, like you used for the other SVGs. That way, you can remove the decorative elements from the HTML structure. Additionally, you should have no text in spans alone. Always use meaningful elements, like paragraphs, headings, lists etc. And finally, you need to look up how to write good alt text. You shouldn't be using words like image or hyphenated phrases as if it's code, it is a human-readable description.
Marked as helpful - @ArifKhanEver@macdeesh
Hello ArifKhanEver,
That looks pretty good except for a few things:
1- Wrap the Equilibrium or the NFT image with anchor tag. It has :hover state, which means that it should be an interactive element.
2- Also, I would recommend using pseudo-element and background properties to put the SVG icons inside your CSS. That way, you can remove the decorative elements from the HTML structure.
- @mustafafaqiry@macdeesh
Hello Mustafa,
You need to make some change in the HTML:
1- Think about when you use the Web. What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. That is essential to fix.
2 - Your Article needs heading. Consider using h2-h6 elements to add identifying headings to all articles. Or I suggest using <div> instead of <article>.
3- Don't forget to write a good alt text to your image.
4- For the CSS, I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other SVG icons you can put it inside pseudo-elements like ::before or ::after, as the icons are all decorative.
Marked as helpful - @OmrZubi@macdeesh
Hello Omar,
That looks pretty good except for a few things:
1- What do you expect when you see a hover effect? You know something is clickable, right? That means it's an interactive element. Every place you see a hover style, you need to include an interactive element, like an anchor tag or button. That is essential to fix.
2- I would do the hover effect on this using pseudo-elements rather than adding extra in the HTML, but that's not essential. Also, for the other SVG icons you can use pseudo-elements like ::before or ::after, as the icons are all decorative.
3- Font size should never be in px, always rem (or rarely em when you want it to inherit from a parent)
4- I would recommend using h1 instead of h3, as the page should contain a level-one heading.
5- Don't forget to write a good alt text to your image.
Marked as helpful
