Do you think my implementation for displaying the image is correct?
Please go through my accessibility and let me know if there are ways I can improve it
Feel free to comment on the accessibility and tips on how I can improve it
What do you think about the accessibility aspect of my code? Please suggest improvements
What do you think of the accessibility of my solution? (especially related to summoning and dismissing the tooltip)
HTML code snippet
...
JS code snippet
function toggleShareButtonAttributes() {
if (tooltip.classList.contains("invisible")) {
shareButton.removeAttribute("aria-describedby");
shareButton.setAttribute("aria-expanded", "false");
} else {
shareButton.setAttribute("aria-describedby", "tooltip");
shareButton.setAttribute("aria-expanded", "true");
}
}
Feel free to comment on my code and suggest improvements especially on code structure and accessibility
Most challenge I have with styling list. To be aligned to left side of parent and have space between marker and text.
What specific areas of your project would you like help with?If I made some mistakes, I probably don't know about them. So please look at my solution and be very critical.
Hello @KonradJam
I am peer-reviewing your code as per the request from FrontEndMentor team
What you did well:
<strong>, <time>.Suggested improvements:
<img src="./assets/images/image-omelette.webp" alt="A golden-brown omelette folded with fresh vegetables" width="375" height="171">
recipe-header-title for the <h1> and you have defined all the stylings within that class. In this case, you can just use the <h1> for styling. The class is not needed in this case. Elements have a lower specificity than classes. It is better to aim for lower specificity as much as possible. There is only one <h1> so the class is not necessary. Lower specificity is better for maintaining the styling and help the overall performance.h1 {
font-family: 'Young Serif', serif;
font-size: 2.25rem;
font-weight: 400;
line-height: 2.2rem;
margin-top: 2.5rem;
margin-bottom: 1.25rem;
color: var(--stone-900);
}
@font-face {
font-family: "YoungSerif";
src: url("./assets/fonts/young-serif/YoungSerif-Regular.ttf")
format("truetype");
font-style: normal;
font-weight: 400;
font-display: swap;
}
@font-face {
font-family: "Outfit";
src: url("./assets/fonts/outfit/static/Outfit-Regular.ttf") format("truetype");
font-style: normal;
font-weight: 400;
font-display: swap;
}
@font-face {
font-family: "Outfit";
src: url("./assets/fonts/outfit/static/Outfit-SemiBold.ttf")
format("truetype");
font-style: normal;
font-weight: 600;
font-display: swap;
}
@font-face {
font-family: "Outfit";
src: url("./assets/fonts/outfit/static/Outfit-Bold.ttf") format("truetype");
font-style: normal;
font-weight: 700;
font-display: swap;
}
I customized and used my social media instead of what the project provided. One of the small things I liked about my coding was making my social media open in another tab or window. I wanted to detail these minor things.
Hi, @DetDet91 good work on this challenge. Good attempt at making the HTML semantic, especially the portion related to the list items. It is impressive that you have gone above and beyond to include the focus style for the buttons.
I have the following feedback for your code:
</article><p> is fine for the text "Beginner front-end developer", you can also explore the <q> tag since it automatically adds the quote.height: 100vh. The fix: Modify it as min-height:100vh because the former is setting a fixed height for the body and hence it will chop anything if the content on your screen exceeds the viewport. min-height:100vh means that the body will have a minimum height of 100vh and hence it will let you scroll vertically. Additionally, consider providing padding on the body so that there is a breathing space around the card.<p> tag is better suited for location. Headings usually introduce content. If you have h2 for location, the expectation is that the remaining content might talk more about the location. But, that's not the case. It is especially easier for users who rely on assistive technology to understand the content of your site. Using the wrong HTML tag can give a wrong impression.Hope the above suggestions help. If you need any clarifications, please let me know.
Hi @kimkim200949, to answer your question on the comment that you left for me at My blog preview component
I suggest the following changes for the #img in your CSS:
max-width: 100%; display:block; should be under your CSS resets. More on CSS resetsFinal definition of the #img after the suggestions
#img {
max-width: 100%;
display: block;
border-radius: 2vh;
}
Does this help?
Your solution is using rem for font-sizes, which is great for scaling. You have incorporated an animation on the hover state by having a duration of 0.2s. You have a good understanding of the flexbox
I would like to suggest the following improvements:
<time> element instead of the <p> for the date. <p> element is more suitable for paragraphs, which normally spans across at least 2 lines. Similarly, the author's name can be represented as <span> instead of <p><h3>. Normally, assistive technology would expect a <h1> tag to understand the flow of the document. What I would do is have an <h1 class="visually-hidden">Blogs</h1> and then wrap the HTML & CSS foundations under <h2> element. You need to write the CSS definition for visually-hidden class. You can refer to visually-hidden CSS definition Reasoning: The hidden <h1> will ofcourse be hidden for users but an assistive technology will read it out and will convey the structure of the page to users who rely on assistive technology, thus enhancing their user experience. The reason for using <h2> instead of <h3> is because in accessibility checks, one should avoid skipping heading levels.:root. You can define other properties like font sizes, line heights, spacings, border radius as well. You can refer to CSS Definitons under rootbody element. You should change to min-height:100vh instead of height:100vh. Then you should apply a gap to have a space between the card and the attribution. Also, you do not have to use display:grid on the body. A simple display:flex should be enough instead since flexbox is appropriate for one dimensional view. Grids can be handy for 2-dimensional layout, which is not the case here.html in your CSS. Refer to this article Should I change the default HTML font-size to 62.5%?figuring out how to start my first project and using git and git hub and trying to figure things out instead of just using tutorials also centering using CSS and not just spamming
What challenges did you encounter, and how did you overcome them?i encountered a challenge that the border radius (rounded corners) were not applying to the top of the QR code i figured out i must use margin and not padding also figuring out how to center was difficult but i used these align-items: center; justify-content: center; height: 100vh;
What specific areas of your project would you like help with?more feedback on the semantic side as i don't know how to do that effectively also maybe tell me if any of my code could be done more effectively
Hello @VADER900000
Kudos on overcoming the challenges related to centering and applying the border-radius.
I have the following suggestions for the HTML and CSS:
<img src="images/image-qr-code.png" class="Qrcode" alt="This is a Qrcode" height="288" width="288">
QrCodeMainContainer. Instead, you can have the styles like background-color, box-shadow etc styles in the qrCodeContainer class instead. Also, apply padding: 1.25rem; text-align: center; max-width:300px; to the the qrCodeContainer class. Let's aim for simplicity.<br> in your code. It is not needed. You can achieve the spaces using margin-top property instead.aria-describedby to the <img> element and have the corresponding id to the <p> element since the image and the paragraph have a connection. It will be easier for users who rely on assistive technology to form a connection between those two<img src="images/image-qr-code.png" class="Qrcode" alt="This is a Qrcode" width="288" height="288" aria-describedby="qr-code-instructions">
...
<p id="qr-code-instructions">....</p>
.topParagraph class. I would instead define them in h1 since h1 has a lower specifity than .topParagraph class. Elements have lower specifity than classes. Lower specificity is better for reusability, maintainability, scalability..topParagraph class since we are using max-width on QrCodeMainContainer class and applying width and height attributes to the <img> element. Similarly, the <p> element does not need width and margin CSS definitions.@media only screen and (max-width: 600px) Hope these tips help. If you need further clarifications, please ask
I'm happy about the banner images and how I used background properties to deal with them. Not sure if it was the best approach or I should have added them using html.
I would like to write better css next time and have a proper arrangement for scss.
What challenges did you encounter, and how did you overcome them?I was struggling with finding out the best approach to add the banner images. I thought maybe I should add them using or maybe image-set but I wasn't sure and I wasn't able to achieve what I wanted so I just went with adding them using url() with background-image and then using media query to replace the image.
What specific areas of your project would you like help with?I would like help with this banner project thing what would have been the best apprach. Like should i have added them in html and then used media queries or maybe use or srcset or image-set?!
Hi @itsmesrishti
I have reviewed your code, and in my opinion, the way you implemented the banner image is very well done. Given that the image is slightly cropped at the sides, using url() with background-image and background-position is an appropriate approach.
I have the following suggestions for the CSS part:
