kasemkl• 40
@kasemkl
Submitted
Feedback on this will be great and any feedback on how i can improve this solution will really be helpful. any feedback is appreciated thank you all guys
Mushfiq Rahman
@Ayon95
All comments
- Mushfiq Rahman• 300
@Ayon95
Posted
Hi, good job completing this challenge. You can make some improvements to your HTML markup and CSS:
HTML
- Try to use semantic HTML elements whenever you can.
- Use the <main> element for the main content of the page.
- An <article> element is appropriate for a piece of content that makes sense on its own, for example, a card like this.
- You can use an <ul> and <li> elements for the card stats section (0.041 ETH, 3 days left).
- A <footer> element is a good choice for the author information section; it'll be like the footer of the article.
- The attribution section should go inside a <footer> element; this will be the footer of the entire page
CSS
- Instead of px, use responsive units like rem and em. These units make it easier to implement responsive web design and they are good for accessibility as well.
- It's good to use CSS variables (custom properties) for values that you use in multiple places in your stylesheet. This will make your CSS code easier to maintain.
Some resources
Hope this helps and good luck on your coding journey :)
Marked as helpful
2 - Iva• 120
@Ivuska
Submitted
Hello from almost sunny Prague!
I have updated my solution of order summary component with the advices and tips from the community's feedback.
Hope it is better now.
Keep coding!
Mushfiq Rahman• 300@Ayon95
Posted
Hi, your solution looks good. It's a bit tricky to guess the dimensions of elements accurately from images alone. There's a screenshot-taking software called Greenshot that you can use to measure distances and such. The PerfectPixel Chrome extension is also useful. It allows you to put an image overlay on top of the coded website. You can use it to compare the design with the actual website.
As for good accessibility practices, use semantic HTML elements appropriately whenever you can. Get familiar with some commonly used ARIA attributes and when they are necessary (MDN has an article on each of them). If you're using Windows, you can download NVDA screen reader to test how a screen reader reads your website.
- PerfectPixel
- Greenshot
- W3C Accessibility Tutorials
- The A11Y Project
- ARIA Attributes
- NVDA screen reader
Hope this helps :)
Marked as helpful
1 - Florian• 50
@Flojo994
Submitted
Hello there,
happy to get some feedback on this one. Especially the overlay. I couldn't get to work it 100%. The Icon is not at 100% opacity. Can't get it to work without the background also completly covering the image.
cheers. Flo
Mushfiq Rahman• 300@Ayon95
Posted
Hi, good job completing this challenge. You can improve the html markup by using semantic elements like <main>, <article>, <ul>, and <li>. The main content of the page should go inside the <main> element. Instead of <div>, it's better to use <article> for the card component. The <article> element is used to mark up a standalone piece of content that makes sense even when it is taken out of the context of the page.
Also for titles that are also links, you should put the <a> element inside the heading element.
As for the image overlay, you can use a ::before pseudo-element on the image container. Since the overlay has an icon, you can specify that icon in its 'content' property. To center the content, you can use flexbox.
<body> <main> <article class="card"> <div class="card__header> <img class="card__image" /> </div> <h1 class="card__title"><a href="#">Equilibrium #3429</a></h1> <p class="card__description"></p> <ul class="card__stats"> <li class="card__stat"></li> <li class="card__stat"></li> </ul> </article> </main> <footer></footer> </body>.card__header { position: relative; } .card__header::before { content: url("./images/icon-view.svg"); display: flex; justify-content: center; align-items: center; position: absolute; top: 0; left: 0; width: 100%; height: 100%; background-color: hsla(178, 100%, 50%, 0.4); }Marked as helpful
0 - Anosha Ahmed• 9,340
@anoshaahmed
Submitted
Would really appreciate any constructive criticism and ways I can improve.
Mushfiq Rahman• 300@Ayon95
Posted
Hey, just wanted to thank you for the way you used background-position to implement the background image as it is in the design. I found it really helpful and added it to my toolbox. Earned a follow :)
1 - Meshach Aderele• 40
@CMconsults
Submitted
-
The elements within my card is inheriting the background color of the body instead of having the background color of the card. I need help on how to fix that.
-
I would love to know the best way to center my card on the screen. I don't think the way I did it here is the best.
Mushfiq Rahman• 300@Ayon95
Posted
Hi, good job on the solution. The problem with background color is happening because you used the universal selector (*) to select all elements and set a background color for them. Instead of doing that, set the main background color on the body element only.
body { background-color: hsl(217, 54%, 11%); }You can use flexbox to center the card within the body element.
body { min-height: 100vh; display: flex; justify-content: center; align-items: center; }By the way, you can get rid of some of the accessibility issues by specifying alt text for the images, and using semantic HTML elements like <main>, <article>, <footer>, <ul>, and <li>.
<body> <main> <article class="card"> .... (html code here) <ul class="currency-area"> <li class="currency-area-1> </li> <li class="currency-area-1> </li> </ul> <footer class="creator-info"> </footer> </article> </main> </body>Marked as helpful
0 -
- Carlos Tudeich• 80
@CarlosTudeich
Submitted
Hey! Thank you for reaching out. Idk how to do that on the image, i mean, i know but idk if putting an empty div is a good practice. Please if u see there is a good change to do let me know!
Mushfiq Rahman• 300@Ayon95
Posted
You can wrap the image in a <div> and use a ::before pseudo-element to create the image overlay.
<div class="image-container"> <img src="" alt=""> </div>.image-container { position: relative; } .image-container::before { content: ""; position: absolute; top: 0; left: 0; width: 100%; background-color: hsl(274, 100%, 34%); opacity: 0.5; filter: brightness(80%); }These are the styles that I used to create the purple image overlay.
Marked as helpful
0 - Anosha Ahmed• 9,340
@anoshaahmed
Submitted
Would appreciate any advice on where and how to get better. Thank you x
Mushfiq Rahman• 300@Ayon95
Posted
Hi, your solution looks great. Good job on going the extra mile and implementing a rotating card. I looked at your HTML code and noticed that you used a <div> for 'card__side--back'. I think a <section> is a better choice as you have done for 'card__side--front'. The front side and back side of the card are like two sections of it.
1 - Santiago• 140
@santiagosg
Submitted
Any feedback is welcome.
Mushfiq Rahman• 300@Ayon95
Posted
Good job completing this challenge. Your solution looks good. You can improve your HTML markup by using semantic elements like <article> and <section> instead of <div> for the card structure. The entire card component could be thought of as a standalone article and each sub-card within it can be considered a section.
Note that an article should always have a heading. If you don't need to show that heading, you can use CSS to visually hide it. The heading will still be accessible to screen readers.
<article class="card"> <h1 class="visually-hidden">The article heading</h1> <section class="card__item"> <section /> <section class="card__item"> <section /> <section class="card__item"> </section> </article>.visually-hidden { position: absolute; left: -1000rem; width: 1px; height: 1px; overflow: hidden; }Marked as helpful
0 - Daniel• 680
@DanielArzani
Submitted
The piece of text that said "Improve your front-end skills by building projects", should I have put that in a <span> rather than a <p> tag?
Mushfiq Rahman• 300@Ayon95
Posted
Hi, your solution looks good. "Improve your front-end skills by building projects" is the title of this card, so an <h2> or an <h3> tag would be a much better option than <span> and <p>. Also, you can improve the HTML markup by using an <article> tag instead of <section> for the card. The main content of the page should go inside <main> tags. These changes will get rid of a lot of the accessibility and HTML issues.
<body> <main> <article class="card"> </article> </main> <footer> </footer> </body>1 - Rapha445• 100
@Rapha445
Submitted
Hello everyone :) Quick question, would you use rems/ems or px for padding and margins?
Mushfiq Rahman• 300@Ayon95
Posted
Hi, I wanted to add to what Rapha445 said. To make using rem a bit easier and more convenient, you can set the root font size to 62.5% of default browser font size which is 16px. This will cause 1rem to be equal to 0.625 * 16px = 10px. This way, you won't have to do additional calculation when converting between rem and px in your mind. For example, if you want to set a padding of 20px, you can just use 2rem.
html { font-size: 62.5%; } body { font-size: 1.6rem; }0
