Happy to hear any feedback :) I think I need to work on organizing my code more into components, especially typography components.
Also would like to hear how you handle headings for accessibility and semantics - do you only use one h1 per page or one h1 per section? What about other headers?
Hi Leon,
I've just had a look at your page and code, most of it seems to be quiet reasonable. To answer your question, you should basically use one single <h1> element that describes the most important content of a particular web page. That's especially important for assistive technology users and search engines.
Regarding accessibility, there are some changes I would consider:
Adding aria-label="primary" to the <nav> element would make it even clearer to screen reader users that this is the main navigation of the website. Screen readers would announce it as "navigation, primary".
The "Get Started" button should actually be an <a> element that only by design looks like a button. When a user clicks on it, he/she will certainly be redirected to another web page. A button actually enables users to trigger an action or event, such as submitting a form, opening a dialog etc.
The titles right after the button seem to belong to the title "All your files in one secure location...", so I think it would make more sense to change them to <h3>.
As far as the testimonial section is concerned, you could improve the semantics by adding a structure like the following:
<blockquote>
<p>... </p>
<footer>
<cite>Auhtor name</cite>
<cite>Job title</cite>
</footer>
</blockquote>
The social media icons in the footer represent a group of items, so they should be included in a list, too. Also, you should add a label for each link, otherwise screen readers would announce them just as "link", users would get confused. You can add visually hidden text inside each link element or add aria-label with the appropriate content.
Besides that, the web page looks pretty good. Keep up the good work. :)
I'm practicing accessibility and semantic HTML elements. Do you have any comments regarding my use of these elements?
Hi Leon,
I just had a look at your code, most of it looks pretty good. There are only a few changes I would consider to improve accessibility.
Although skipping heading levels should be avoided, I'm not sure if the <h1> heading is appropriate in this case. The card is just a component that would most likely be reused on an existing website. Therefore, <h2> or even <h3> would be more realistic.
As @tmciesco mentioned, the given alternative text for the image would only cause confusion to screen reader users. You also wouldn't label a link with its URL. Something like "QR code, scan to visit Frontend Mentor" would be more clear about what purpose the image has.
As far as the HTML structure is concerned, the most of it looks reasonable, but you may want to replace <section> with <article>. MDN makes it very clear, what purpose this element has:
"The <article> HTML element represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable [...]. Examples include: a forum post, a magazine or newspaper article, or a blog entry, a product card, a user-submitted comment, an interactive widget or gadget, or any other independent item of content."
To be thorough, I would also change the attrbution container to be a <footer> element and take it out of <main>.
I hope I could help. Keep going, and happy coding. :)
Looking to learn about frontend best practices so any feedback is welcome.
Hi Jennifer,
I just had a look at your code, most of it looks pretty good. There are just a few changes I would consider.
To center the card in both directions, a better way is to use Grid or Flexbox instead of position and transform:
body {
display: grid;
place-content: center;
min-height: 100vh;
}
For .card-container it's better to use max-width instead of width as it prevents horizontal scrolling on small screens.
As far as accessibility is concerned, alternative texts for images should have a clear message. Instead of "QR code", it would be better to have "Scan this QR code to improve your front-end skills".
Furhtermore, you should avoid skipping heading tags. The most important heading should always be <h1>, the second <h2> etc.
It's also important to use semantic section elements to allow browsers, assistive software and search engines to understand the structure of your web page. One of the most common ones is the <main> element that contains the main content of a page. Therefore, every web page needs to have a single <main> element. In your case, the card component needs to be wrapped inside <main>. You can easily do this by replacing the parent div tag with main.
I hope I could help. Keep up the good work, and happy coding. :)
For some reason the background images doesnt apper when i upload the site to github pages.
Hi Valentin,
To display the background image, you need to change the URL in CSS as follows:
background-image: url(../images/bg-desktop.svg);
Besides, the alt attribute of the hero image needs to be empty as long as it's decorative, otherwise screen readers would read it as "hash" (#), which would be confusing.
The rest looks pretty good. Keep going, and happy coding. :)
Hi there,
No real problem excepted the fact that I don't understand why I had to set my button's display to "flex", otherwise the text was offsetted to the right. Any answer to this ?
Cheers !
Hi Pierre,
Anchor elements (<a>) are inline elements, so margin has no effect and the layout breaks. You could also change it to display: inline-block and omit the Flex properties along with max-width: 200px;. The result would be the same.
I hope I could shed some light on this. Happy coding. :)
I could not figure out how to code the active state for the image. How do you make those styles?
Hi VMH1225,
To get the hover effect, you should wrap the image like this:
<a class="overlay" href="#">
<img class="mainImage" src="images/image-equilibrium.jpg" alt="Equilibrium">
</a>
And then add the following CSS rules:
main {
isolation: isolate;
}
.overlay:hover {
background: linear-gradient(
hsla(178, 100%, 50%, 0.5),
hsla(178, 100%, 50%, 0.5)),
url(images/icon-view.svg) center no-repeat;
border-radius: 2%;
}
.mainImage {
position: relative;
z-index: -1;
vertical-align: top;
}
If something is not clear, feel free to ask. Keep it up, and happy coding. :)
Having trouble getting footer p to respect the reduced font-size setting @ 475px
Used flexbox mostly..
Hi Nicholas,
I have just looked at your code and noticed that three CSS rules address the <p> element inside <footer>:
.attribution { /* SPECIFICITY: 10 */
font-size: 11px;
}
footer p { /* SPECIFICITY: 2 */
font-size: 4px;
}
@media screen and (max-width: 475px)
footer p { /* SPECIFICITY: 2 */
font-size: 4px;
}
The class selector .attribution will always win because of its higher specificity. You could solve this problem by replacing footer p with .attribution within the media query.
You can learn more about specificity in CSS on Google Developer's web.dev website.
Another tip: Avoid defining media queries with max-width only, as they overlap each other, which again can lead to problems with specificity.
I hope I could help. Keep going, and happy coding. :)
i'm done! 2 days of work! 🤡 I have one question to you guys cause i wanted to do it a bit different but my limited knowledge and reasearch didn't let me do it 🥲 I have a huge question... How can I adjust the height of these three images at the bottom to occupy the exactly same height as the siblings div content?? I was trying to set the height as a 100% regarding to the content container (it's a class = 'article-content').. In other words, first I specified the dimension of the div class = ('article-content') content which is on the right side of the each picture and THEN i wanted to adjust exactly the same height with relative values instead of making my grid container 13fr and 30fr .... 🤡 I would be very appreciated if someone could help me with that! Thanks a lot and have a nice day!
Hey Milosz,
As far as I understand, you want article-img and article-content to be the same height. To do that you could just change the value of align-items to stretch on article-container. This way article-content will always fit the height of article-img.
Hope this solves your problem. Feel free to ask, if something is not clear.
Keep it up, and happy coding. :)
I really enjoyed building this project and found I only had difficulties with one part.
I would love feedback on my mobile media query. How do I get the container to fill the entire frame? I was able to get my solution to match the dimensions provided, but on a smaller device, there would be white space at the bottom. How do I fill the whole screen?
Hi Tricia,
You should add the background to the body:
body {
background: hsl(212, 45%, 89%);
}
For this card component you actually don't need any media queries. Instead, you could use the properties width and max-width to make it responsive:
.container {
width: 100%;
max-width: 400px;
padding-inline: 24px; /* Add padding on the sides */
}
This way, the card takes up the entire width on screens up to 400 pixels wide. You can of course change these values to match the original design.
Hope this helps. Keep going, and happy coding. :)
i found placing the stars icon a bit challenging, please let me know how to do it in the best way, and whether I should put the svg in the HTML file or the CSS file and how to repeat them without losing the count of the stars.
Hi Hasan,
Using background-repeat: space; seems like a reasonable solution to avoid repetitive code. The drawback, though, is that you have no control over the spacing between the icons. A better solution would be to specify background-position for each icon.
Here's an example:
.star-icons {
width: 105px;
height: 20px;
background:
url("../images/icon-star.svg") left center,
url("../images/icon-star.svg") 22px center,
url("../images/icon-star.svg") 44px center,
url("../images/icon-star.svg") 66px center,
url("../images/icon-star.svg") 88px center;
background-repeat: no-repeat;
}
The first value right after the URL defines the horizontal position. The icon has a width of 16px, so in this example 6px is added to the width for the second icon to add spacing (16px + 6px = 22px).
You could also add the SVG code into the HTML document and set the spacing using the margin-left property. Both techniques seem to be alright. The main advantage of including SVG code into HTML, usually called "inline SVG", is that it reduces the number of network requests when loading the page, which is great to improve performance. But in this case it doesn't make a significant difference, as it is only a single icon. Using inline SVGs for a larger number of images, such as logos, social icons and others, makes more sense.
It's up to you which technique you prefer.
Keep going, and happy coding. :)
Hello everyone, I hope that you are doing great. I am back to solve some challenges and I enjoyed this one. If someone have any advice on how I can improve, please comment this challenge. Thank you in advance!
Hey Milos,
I like your solution and the correctly applied HTML structure.
To improve accessibility, though, you should avoid using the <div> element for semantic elements, such as buttons. To make the menu button accessible for keyboard and screen reader users, you should use the <button> element instead .
In addition, the alternative text of the menu icon should describe the purpose of the menu button, i.e. something like "main menu" would be more precise, as screen readers would then read the button as "main menu, button".
You should also place the menu button right after the opening <nav> tag so that users can recognise it more easily.
Here's a simplified example:
<nav>
<button>
<img src="" alt="main menu">
</button>
<ul>
...
</ul>
</nav>
The rest looks pretty good. Keep going, and happy coding. :)
hi all, any feedback for cleaner code is welcome. I have to add some things, like the hover on the buttons of the about section and improve the texts of the same section. There is still a lot to improve, so thanks for any feedback.
Hi Guada,
I just look at your solution and I really like it, especially the fact that the web page is accessible with a reasonable HTML structure. Also the trick with role="list" on <ul> elements to keep the semantic meaning, pretty neat. Frankly, I only recently found out about this bug fix.
There are just a few changes I would consider:
I see that you have added aria-label="Primary" to the <ul> element inside the primary navigation. A better, more appropriate place for it would be inside the <nav> element, to make it clear for screen reader users that this is the primary navigation.
The menu button is an important part of the navigation, so it should belong inside <nav>, otherwise it would be announced seperately by assistive software. Also, the aria-controls property should indicate the <ul> element.
The purpose of the Contact "button" is most likely to navigate the user to the contact page, not to trigger an action or event. Therefore, it should be a link which only by design looks like a button.
With these changes in mind, I would recommend rewriting the navigation part as follows:
<nav aria-label="Primary">
<button aria-controls="primary-navigation" aria-expanded="false">
...
</button>
<ul role="list" id="primary-navigation">
<li><a href="#about">About</a></li>
<li><a href="#services">Services</a></li>
<li><a href="#projects">Projects</a></li>
<li><a href="#contact">Contact</a></li>
</ul>
</nav>
The rest is superb, keep up the good work. :)