falguni-ux• 10
@falguni-ux
Submitted
P
Dave Quah
@Milleus
All comments
Greetings! 👋 Congratulations on completing the challenge.
I have some suggestions for improvements:
IMAGE ALT TEXT
- Consider having a more meaningful alt text for the QR code image. For example,
<img src=“…” alt=“QR code to frontendmentor.io” />.
HEADING STRUCTURE
- Consider having a unique
h1per page that describes what the page is about. Having the right heading levels would help screen reader users and also search engines. More details about heading structure here.
LANDMARKS
- The
<main>landmark is missing, consider renaming the<div>to<main>. Landmarks help to define major sections of your page and can help screen reader users a great deal. More details about landmarks here.
I hope this helps and happy coding! 😊
0- Consider having a more meaningful alt text for the QR code image. For example,
- Aditya Baranwal• 200
@CHARLIEADITYA
Submitted
Greetings! 👋 Congratulations on completing the challenge.
I have some suggestions for improvements:
IMAGE ALT TEXT
- Consider having a more meaningful alt text that explains the purpose of the QR code image. For example,
<img src=“…” alt=“QR code to frontendmentor.io” />.
HEADING STRUCTURE
- Consider having a unique
h1per page that describes what the page is about. More details about heading structure here.
I hope this helps and happy coding! 😊
0 - Consider having a more meaningful alt text that explains the purpose of the QR code image. For example,
- Magdalena• 500
@magsaram
Submitted
I build it with flex, so it's full responsive. I had small problems with it, but finaly I did it! Small thing, but I'm happy ;)
👋 Hello!
Congratulations on completing the challenge. 🥳
There are a few things that I think would make this even better:
- Use only one <h1> per page, for accessibility and SEO. In this case I'd see "Sedans", "SUVs" and "Luxury" of the same logical level, so I'd approach this by adding a <h1> with a screen reader only class (visually hidden but still picked up by screen readers), and have "Sedans", "SUVs" and "Luxury" as <h2>. Here's a good video by Kevin Powell sharing more about headings.
- In the footer, to be more semantically correct, use <p> instead of <div> for texts.
- For the rounded borders, an alternative approach is to set
border-radius: 8pxandoverflow: hiddenon the parent. This could make things a little easier as the border-radius would not need to be updated in the media query. - For the anchor elements CSS, because the font-size isn't explicitly set and there's a fixed pixel height and width set, there will be text overflowing out of the element when the base font-size increases (i.e. someone who increased font size on their device). Most cases of fixed widths or heights could be replaced with
min-widthormax-widthetc, so that it scales better. Another approach would be to usepaddinginstead. I'd also recommend usingemorremunits instead ofpxfor better accessibility.
Nice work and keep up the good job! I hope this helps and happy coding! 😊
Marked as helpful
0 - Mochimooo• 50
@mochimooo
Submitted
Hi Frontend Mentor peers:
- Is there a better way to write the hover effect than how I wrote it in my code?
.button:is(:hover,:focus){ background-color:hsl(245, 75%, 52%, 80%); cursor: pointer; } .hover:is(:hover,:focus){ cursor:pointer; color:hsl(228, 45%, 44%); font-weight: var(--fw-bold); } .hover-1:is(:hover,:focus){ cursor:pointer; color: black; font-weight: var(--fw-bold); }- How can I anchor the footer note to the bottom of the page? I tried using `.bottom{position: fixed; bottom: 5px;' and it would cause the footer note to overlap onto the summary card. 🤷♀️
Thank you.
👋 Hello!
I think using
:isis fine, but I would probably move thecursor: pointerto the base element (non-hover) instead.More importantly though, it would be a lot better if the proper interactive HTML elements, i.e. <button> or <a>, were used instead of <div>, <u> and <b>. Using the proper elements would help with accessibility greatly - able to navigate on with keyboard, pressing "Space" or "Enter" triggers the element, screen reader would announce that it is a clickable button or link, etc.
As for how to anchor the footer note, my approach was to use CSS grid. I'd declare 3 rows (1fr, auto, 1fr) and have the <main> in row 2 and <footer> in row 3. Row 1 is an empty row to help keep <main> centered perfectly (thus why it is 1fr, similar to row 3).
I hope this helps and happy coding!
Marked as helpful
1 - luking• 230
@alululululuer
Submitted
Always use correct heading levels.
👋 Hello!
This is a nice solution - I like the use of HTML elements and CSS custom properties.
I think a few things that would make this even better:
- The "attribution" as a <p> instead of a <div>, and if it could fit in the same viewport without scrolling.
- Addressing the accessibility concerns with Safari browser for
list-style: none - Perhaps consider utility classes for font size, weight and color instead of declaring them with headings (h1, h2). This gives more flexibility as most times headings aren't always styled the same.
Really nice! I look forward to seeing more solutions from you. Happy coding!
Marked as helpful
0 Hi, everyone.
This is my second solution to the project (the mobile first again). I got a huge feedback from Grace Snow and fixed some bugs according to them. I tried to solve some problems with responsive design. I guess this solution is a progress. I know there are lots of issues still here. I hope I can find and fix them. Thanks...
Any comments, critique, advice is greatly appreciated. For those of you reading this, have a nice day!
👋 Hello!
I think one thing that could be improved on is the heading levels. I'd view "Sedans", "SUVs" and "Luxury" as the same heading, but I also understand that we do not want multiple <h1>.
I think it would be better to declare a <h1> but set it for screen readers only (meaning not visible but screen reader would still pick it up), and keep "Sedans" as a <h2> with "SUVs" and "Luxury". E.g.
// CSS .sr-only { position: absolute; top: auto; height: 1px; width: 1px; overflow: hidden; clip: rect(1px, 1px, 1px, 1px); white-space: nowrap; } // HTML <h1 class="sr-only">Learn more about our cars</h1> ... <h2>Sedans</h2> <h2>SUVs</h2> <h2>Luxury</h2>Another suggestion is that for images that are decorative, the alt should be
alt=""instead ofalt="#", or alternatively you could also setaria-hidden="true"to hide it from screen readers.Some useful resources:
I hope this helps and happy coding~
Marked as helpful
1- Bruno Costa• 40
@BrnCosta
Submitted
This was my first time using Tailwind in a project
Any tips and feedbacks are more than welcome.
Thank you!
👋 Hello! Congrats on completing the challenge.
Tailwind actually comes with preset font-sizes, I'm curious why not use those instead? e.g.
text-xswhich is 0.75rem (12px),text-smwhich is 0.875rem; (14px), etc?Alternatively, font-size could be overwrote with your own set of sizes via
tailwind.config.js.Also, one tiny trick that could help with writing lesser code is to use the
gap(flex and grid) or thespace-yandspace-xutilities. They help to add gaps or margin between child elements so you wouldn't need to keep adding margins to every child element 😊Hope this helps and happy coding~
Marked as helpful
0 - Bel030• 10
@Bel030
Submitted
I had trouble centering the div without affecting the format of the text and image, along with centering the div itself rather than being located on the top-left corner of the body. This was resolved by implementing a margin to the div.
👋 Hello! Congratulations on completing your first challenge.
Typically you'd want to add a
min-height: 100vhto the body so that the minimum height stretches to match the viewport. After that we could use Flexbox or Grid to help with centering. E.g.body { display: flex; align-items: center; justify-content: center; }or with grid
body { display: grid; place-items: center; }I'd suggest reading up more about Flexbox and Grid. MDN website is a good source for learning. There are also Flex/Grid game websites to help with learning.
Other than the issues highlighted in the accessibility report, one other thing that could be improved is to change the
<div class="attribution"to a <p> tag instead as it would be more semantically correct.Hope this helps and happy coding~
Marked as helpful
1 - Kierro DeCampanille• 130
@Kirp
Submitted
Had a rough time with properly placing the image and trying to figure out a non js solution to the accordion effect. In the end it felt really wonky to have a form just to record if its clicked so I just went with using js.
Hello! Congrats on completing the challenge. I recall having a rough time with the image placements as well heh..
Anyways, I wanted to share that one of the non-JS solutions is to use HTML <details> and <summary>.
But this requires a little extra CSS to remove the disclosure triangle for webkit-based browsers.
Hope this helps and happy coding~
Marked as helpful
0 - Nikolas Papenfus• 380
@NPM0486
Submitted
Are the class names appropriate? Did I give too many div tags? I had a problem with the hover on the photo, I currently made a div that is above the photo, but in the plans I had to use :after but it didn't work so I went to plan B.
Hi Nikolas, congrats on completing the challenge 🥳
There seem to be an issue where when the mouse is at the center of the picture (where the invisible "eye" is), the overlay and eye disappears. I believe this is because in CSS the effect triggers on
.overlay:hover, but when the mouse is in the center it is hovering over the invisible <img class="view"> instead of the <div class="overlay">.One way to fix it is to set
pointer-events: noneto the <img class="view">, https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events.Another way to fix this is to remove
<img class="view">and add it as a background-image to the overlay instead.I hope you wouldn't mind if I share my solution with you. It uses
background-imageas well as:after- https://www.frontendmentor.io/solutions/nft-preview-card-component-rJRIRaSVcHope this helps and happy coding!
Marked as helpful
0 - Martin Amankwaa• 40
@MartyOfMCA
Submitted
Could anyone kindly explain you to what I'm doing wrong in setting the font family for my title (content-title in my markup file) to Fraunces.
I humbly welcome suggestions on any other areas I missed. Thank you!
Hello,
The font import URL is incorrect. Go to the font links and select the font weights/styles, a side window will pop up with the selected fonts and guide you on how to import the fonts.
- https://fonts.google.com/specimen/Fraunces
- https://fonts.google.com/specimen/Montserrat
The import should look something like this
@import url('https://fonts.googleapis.com/css2?family=Fraunces:opsz,[email protected],700&family=Montserrat:wght@500;700&display=swap');Hope this helps!
Marked as helpful
0 - Ranjana Mukhia• 150
@ranjanamukhia
Submitted
I want the feedback on following things:- 1.Please let me know what can be done to make it responsive as iam not sure how the white card having blue-background and persons photo should come up in smaller screen size than 340px.
Hi @ranjanamukhia 👋
Congrats on completing the challenge 🎉
To your question on how to fix accessibility issues:
-
The page needs a level-one heading, meaning it needs a
<h1>element. More information here. If the existing design does not have an appropriate text to be a h1, you can add your own h1 title and make it visibly hidden but still picked up by screen readers. -
The
<div class="attribution">needs to be within a landmark, landmark refers to elements such as<header>,<nav>,<main>,<footer>. There are several ways to fix this, either move the <div> into the <main>, wrap it within a <footer>, or changing the <div> to a <footer> since it makes sense in this case. More information here.
Hope this helps, happy coding!
Marked as helpful
0 -
