Gwenaël Magnenat
@gmagnenatAll comments
- @IsaacKelSubmitted 10 days ago@gmagnenatPosted 9 days ago
Hi, congrats on giving a try to this challenge !
I noticed a few issues in your solutions so here is a list of points to check out that will help you understand how to build more robust solutions.
- Use an external stylesheet that you call in the <head> of your html.
- Add a full modern css reset at the top of your stylesheet in all your projects. Check out Andy Bell and Josh Coeau. They both have a good reset file. Pick the one you like.
- Organise your stylesheet in a visual order following the design. Try to go top to bottom, it will help you and other developers read your code.
- Never use pixels for font-sizes. Read this Why you shouldn't use pixels for font-size and this The surprising truth about pixels and accessibility
- Use a max-width in
rem
for your card instead of a fixed width in pixel. This is important, similarly to the font-size issue. Some users increase the default font-size of their browsers. If you are using pixels you cannot respect these settings and your sizes will not scale. - use a min-height:100svh on your body so the content will not crop if it need to take more places when it's zoomed in.
- You can remove the width and height on your image as it's contained in the card, you just need a padding on your card. A value of max-width:100% will be included in any modern css for images. You can add a width:100% in addition for when the image needs to take more space then it's original width.
- A <main> landmark is missing. It's important to indicate the main content of the web page. important, it should not be your card component.
You'll have something like :
<body> <main> <div class="your-card">...</div> </main> </body>
I hope you find something useful in this list to improve your understanding and help you make better solutions.
Don't hesitate to reach out on discord if you have any questions.
Happy coding!
Marked as helpful2 - @vardhan-venkataSubmitted 16 days agoWhat are you most proud of, and what would you do differently next time?
In this i have learnt a new thing called text-shadow
What challenges did you encounter, and how did you overcome them?I faced challenge in achieving the text-shadow, and then finally i achieved it.
@gmagnenatPosted 16 days agoHello, congrats for giving this one a try !
There is a nice list of concepts that needs to be understood on this challenge. It will help greatly later to build responsive solutions with more complex layouts.
- You need to add a <main> landmark to indicate the main content of your page.
- alt tag should not contain words such as "Image" it is already announced by screenreader as an image. It should describe the image and it's purpose. So here it's a QR Code and the purpose is "going to frontendmentor io".
- As this is a small component that would sit on a larger project it will probably not use an h1 as the main page title. You can use an h2 here and ignore the automatic validator.
- Use a modern css reset at the top of your stylesheet in all your projects. Look up for Andy Bell or Josh Comeau, they both have great ones. Use the one your prefer and try it out. It will help you deal with browser inconsistencies and you will start your css on a strong foundation.
- use a min-height instead of a fixed height on your main class
- use a max-width in
rem
on your qrCode class, and remove the height. - remove the width and height on the image. It is contained in the card so you can just have a small padding on the card and that's all.
- never use pixels for font-sizes https://fedmentor.dev/posts/font-size-px/
I hope you find something helpful in these comments to improve your solution and your next ones.
Don't hesitate to reach out on discord with your questions.
Happy coding !
0 - @ZainabeyySubmitted 16 days ago@gmagnenatPosted 16 days ago
Hello, I'm afraid there are issues in this solution and it needs more work to be accessible.
If you try now with a screenreader, you'll notice the first element announced is the title and then the icon and success message. Nothing else is announced unless I press tab to go to the first form input. If I accidently press enter, no error message are announced so I have no idea what is going on.
There are also some fundamentals concepts that are not covered, it should be all understood at this challenge if you take the learning path and get good feedback on your easier challenges.
Here is a short list of pointers to check up.
- you need a <main> landmark to indicate where the main content of the page is.
- it's more performant and advises to load the font from your HTML instead of your CSS
- use a modern CSS reset at the top of your stylesheet in all your projects
- I would use an eventListener for the submit event instead of an onsubmit, cleaner and more modern.
- you need to add some aria for the errors. I found this article interesting and covering of lot of use case https://www.smashingmagazine.com/2023/02/guide-accessible-form-validation/
I hope this helps you understand more what is needed for an accessible solution.
Don't hesitate to reach out with your questions on the discord.
1 - @ChiangArtSubmitted 28 days agoWhat are you most proud of, and what would you do differently next time?
uhmmm, a medida que voy avanzo, quiero mejorar mi codigo y usar otros frameworks
What challenges did you encounter, and how did you overcome them?ninguno
What specific areas of your project would you like help with?por ahora ninguna
@gmagnenatPosted 24 days agoCongrats on giving this challenge a try !
Here are a few points that you'll want to check, it covers some fundamentals tips for building responsive and more accessible solutions later.
- you need a <main> landmark to indicate the main content of the web page
- you should load your stylesheet after loading the font styles, not before.
- adding a modern css reset at the top of your stylesheet in all your project will help you deal with browser inconsistencies and have a strong foundation for your stylesheet. Look up for Andy Bell and Josh Comeau, they both have solid ones.
- using pixel for font-size is an accessibility issue as it cannot respect the user browser settings. Use
rem
instead. https://fedmentor.dev/posts/font-size-px/ - 100vw on your container is not advises as it can cause overflow in some case. You can remove it.
- use a min-height:100vh instead of a fixed height on your container. It should be able to grow with the content.
- use a max-width in
rem
for the width of your component instead of the fixed width in pixel. - the links should not have a width but take the 100% width of the card. Add a padding to the card so it doesn't touch the edge
I hope you find something useful in this list. Don't hesitate to reach out on discord if you have any questions.
Happy coding !
0 - @TheWraithDevSubmitted 29 days agoWhat are you most proud of, and what would you do differently next time?
I would use semantic tags more - I feel I did pretty well in the CSS which is usually my downside
What challenges did you encounter, and how did you overcome them?I struggled with the Figma file initially - however after some thorough research it got a lot easier on how to use - still a beginner but getting there.
What specific areas of your project would you like help with?I would like to get better at responsive designs more - I want to understand responsive layouts more and when we can use them etc
@gmagnenatPosted 27 days agoThis is a duplicate of this one : https://www.frontendmentor.io/solutions/responsive-qr-card-component-3oZOfBeIPr
My review stay the same, there are no differences.
0 - @EvertxsSubmitted 29 days agoWhat are you most proud of, and what would you do differently next time?
I think this was a fairly basic one but it feels nice to be able to know the bare minimum that I do and replicate it
What challenges did you encounter, and how did you overcome them?I still struggle with aspects of width and height / length, it's hard to understand what takes up what space
What specific areas of your project would you like help with?Is my project responsive? Does it match the mobile layout?
I would also like assistance in the height x width/length topic
@gmagnenatPosted 27 days agoHello, congrats for giving this challenge a try!
Here are a few things you may want to check before moving to other challenges:
- You need to have a
<main>
landmark. It indicates where the main content of the webpage is. - You should read about how to write good alt text. There are great resources for this on the Discord. For example, you should avoid using words like "image" in an alt attribute. It needs to describe the image for users who cannot see it, as they already know it's an image.
- Remove the
width
andheight
attributes you have set in the HTML on the image. In some cases, the image cannot adapt to fill its parent width and will stay at the288
value you have set. - It looks like you are confused about when to use margin and padding. Here is a good article about it: https://fedmentor.dev/posts/padding-margin/.
- It is a good practice not to style your HTML elements directly but to use a class on elements you want to style. Try to keep the specificity at the lowest level and only increase it when really necessary.
- Having an
<h2>
for the title is fine here as this is a single component and not a full page. If you put this component in context it would probably sit on a larger page with other components.
2 - You need to have a
- @Tonyac-createSubmitted about 1 month ago@gmagnenatPosted about 1 month ago
Hi, congrats on taking this challenge!
I notice some important points you'll want to fix. It will help you and make your life much easier when styling and later when using JavaScript.
Does the solution include semantic HTML?
- It's better to load your stylesheet after the fonts. The risk here is loading some styles without the right fonts.
- You need to read about landmarks and when to use which (
<main>
,<header>
,<footer>
).<main>
represents the dominant content of the page.<header>
and<footer>
need to be outside of<main>
. If you add them in the main landmark, they act like adiv
and are completely useless. Each of these landmarks has an implicit role when they are used right. A header landmark has a "banner" role and contains content that is repeated on different pages such as navigation. There is no need for a header in this solution. As the footer needs to be outside of<main>
, it can usually contain the attributions and links in these challenges. Otherwise, you don't really need a footer on this component. - About the use of
section
: it represents a section of a document and needs a heading. As you only have one here and don't need to differentiate several sections, adiv
is perfectly fine. - The main title of the component is the product name, so this should be your heading. For the perfume overline, you can use a
<p>
tag. Also, I recommend you don't use capital letters in your HTML but style this with CSS to put the text in caps. As it is a component that will probably be displayed on a page with other similar components, it's better to use anh2
, as theh1
will be in another place on that page. You can ignore the HTML validation on the form or use a hiddenh1
if you want. - For the price part, if you are a screen reader user, this part is confusing as it announces two prices without a difference between them. You can use the
<del>
element to wrap the previous price and also add a screen reader-only text on that element that says, for example, "old price was:". It adds more context to what is announced to non-sighted users. - The image on the button is purely decorative and doesn't add more information to this button for non-sighted users. You need to leave the
alt
text empty so the screen reader skips that icon and doesn't announce it.
Is it accessible, and what improvements could be made?
- You need to consider users who are changing their browser's default font size. The default font size of the browser is 16px, but some users change this value to display websites with larger text. You can read this great article that explains why you should not use pixels for font sizes. It gives examples and techniques on how to test your code.
- On the same topic, you always want to use relative units such as
rem
for your media queries. As the user can change the default browser font size, you want to respect these settings and switch your layout at these updated values. Pixels don't let you do that. This counts for your media query but also for the switch of yourpicture
element. - You need to add a modern CSS reset at the top of all your stylesheets in every project. Look up Andy Bell or Josh Comeau; they both have great modern CSS resets (and also super great blogs). Make sure to understand what each line of these resets does. It will help you write strong styles from the start instead of fighting with some browser inconsistencies.
- You can remove the height of 100% on
body
andhtml
. It's already 100% height and width by default. - It is recommended to use a unitless value for
line-height
to avoid unexpected behavior unitless line height: MDN. - You should use relative units on your
min-width
such asrem
. If you test your solution by increasing the font size of your browser to 40px, you will see that your solution breaks. It needs to respect the user's settings. - Instead of using
!important
values, you can useflex-box
,max-width
on your image, and other techniques to have a more flexible and solid solution.
Does the layout look good on a range of screen sizes?
- It looks good and respects the design. Some areas can be improved for better accessibility.
Is the code well-structured, readable, and reusable?
- I recommend you structure your CSS from top to bottom with different sections. Currently, it's a bit unordered, going from button to main, then picture, and so on. Try to follow the visual order of the design and structure your stylesheet in that order. It will help others read and understand your code quickly.
- Try to focus on the lowest specificity of your selectors and increase only if really needed. Add classes to every element you want to style. It will help you build a more maintainable codebase. If tomorrow you change the HTML, the stylesheet stays the same, but if you style directly on the HTML elements, you need to rewrite the selectors at each change.
Does the solution differ considerably from the design?
- It looks good, good job! Just the important things about HTML you want to check. You are overcomplicating stuff currently. It can be much cleaner, more solid, and accessible.
I hope you find some useful informations and tips in this review and I hope it helps you better understand that good HTML is a very important fundamental skills that helps build solid and accessible solutions.
Let me know if you need more clarifications on one of the topic.
Happy coding !
1 - @bmch09Submitted about 2 months agoWhat are you most proud of, and what would you do differently next time?
I am proud of myself for having a lot of desire to keep learning and to be able to create this website.
What challenges did you encounter, and how did you overcome them?There are some things I can't figure out, but with practice I get there.
What specific areas of your project would you like help with?.
@gmagnenatPosted about 2 months agoHi,
Congrats on giving this challenge a try! Here are a few tips to improve your solution:- Don't use "image" in alt text. There are great resources on the Discord about how to write good alt text.
- You can use an
<h2>
instead of<b>
for the preparation time. - Use
<strong>
in your list instead of<b>
. It's used here as a label for the line, so it has semantic meaning. - You need to add
<th>
in your table to indicate the table head. - Use a modern CSS reset at the top of your stylesheet in all your projects. Look up Josh Comeau or Andy Bell—they have good ones. Make sure to understand what each line in these resets does and choose the one you prefer.
- Width and height of 100% on your body are not necessary.
- Use a
max-width
inrem
instead of pixels on your<main>
element. It needs to respect the user's preferences if the default font size of the browser has been increased. - I notice that you are not building this mobile-first.
- Work mobile-first, and add a media query when you switch to a desktop layout with a
min-width
value inrem
. - You should import your fonts in your HTML instead of the stylesheet for better performance.
- You can move the title in the
<head>
, just under the meta viewport tag, also for better performance.
I hope this helps you improve your solution and your future challenges. Don't hesitate to reach out on the Discord server if you have specific questions.
Happy coding!
0 - @bmch09Submitted about 2 months agoWhat are you most proud of, and what would you do differently next time?
to do it more faster
What challenges did you encounter, and how did you overcome them?.
What specific areas of your project would you like help with?.
@gmagnenatPosted about 2 months agoHello,
There's a lot of great comments and resources linked above.
Here are a few tips to improve the accessibility of your solution:- Use a
max-width
inrem
for your card width instead of a pixel value. It needs to respect the user's preferences if they increase the default browser font size. - Use a
min-height
of100vh
(or100svh
) on the<main>
element instead of a fixed height to avoid content cropping in case the font size is increased.
I hope this helps.
Happy coding!
Marked as helpful0 - Use a
- @gideonakpanSubmitted about 2 months agoWhat are you most proud of, and what would you do differently next time?
Working with JSON
What challenges did you encounter, and how did you overcome them?Simplifying the code
What specific areas of your project would you like help with?None
@gmagnenatPosted about 2 months agoHello,
I'm sorry, but there are a lot of issues here.
First, everything should be usable with a keyboard. On your solution, nothing receives focus when the Tab key is pressed.
Every interactive element, such as links, buttons, etc., should use a focusable HTML element such as an anchor
<a>
or button<button>
.You shouldn't add a hover style or cursor pointer to something that is not using the correct HTML focusable element. It indicates that the element is interactive, so it needs the correct semantic element as well.
There are different elements that can be focusable in this layout:
- The tabs (Daily, Weekly, Monthly)
- The card
- The "..." in the card
This is a tabs layout and should follow the tabs pattern.
You should use
rem
orem
in your media queries to avoid screen overflow. It can happen if the user is increasing the browser's default font size. The layout shift needs to follow the user's preference.There is a lot to fix in this solution, and it's definitely not a beginner challenge, so I would probably recommend you do simpler responsive layouts first.
I hope this is not to discouraging but let me know on discord if you have specific questions.
happy coding !
Marked as helpful0 - @ajibonaSubmitted 8 months agoWhat are you most proud of, and what would you do differently next time?
Im so happy to have complete these project and make it responsive what really makes me happy being part of these challenge and finding a solution to it i feel like i did something great. i can"t wait to try another challenges and learn something new
What challenges did you encounter, and how did you overcome them?i have always find it really hard to make my own exactly like the design i try spending some couples of hours on it and try make sure it looks exactly like the design and im so happy how it later turns out. making it responsive its really im so happy i faced it all
What specific areas of your project would you like help with?Any feedback will be ok its keep me learning and stay on tracks.
@gmagnenatPosted 2 months agoHi, congrats on giving this challenge a try!
Here is a list of things I noticed that could be improved to enhance your solution and possibly your other projects:
- It would be great to have a personalized README. There is a template in the project files. Remove what you don't need and add more info about the challenges you faced and the good things you learned. Having a good README in your project can be very beneficial for someone reviewing your GitHub profile. It creates a good impression.
- Remove the elements you don't need from your HTML.
- You can simplify your HTML by removing this inner section. I understand you used it for the black border animation, but you can achieve this with a pseudo-element on your card.
- The card image is meaningful and is related to the blog topic, so you should not use a background image here. Instead, use an
img
element with a properalt
attribute. - You should not skip headings. Use another HTML element if necessary, but keep them in order (h1, h2, h3, etc.). This helps with understanding the page structure and content organization. The date is not a heading. The author’s name is not a heading.
- This is a preview card leading to a blog post, so you need a link in the heading.
- You don't need to wrap everything in a section; it's not necessary here. You already have the
<main>
landmark, so you can go straight to your carddiv
. - You need to add a modern CSS reset at the top of your stylesheet in all your projects. This will help you focus on clean code and ensure better cross-browser support. Look up for Andy Bell's css reset or Josh Comeau.
- Use
min-height
on yourbody
, not just a simple height, as content can grow if the user increases the font size. - You should not use a fixed height for elements containing text. The amount of text can vary, and the font size might be increased by users who need larger text.
- You don't want to use a fixed width in pixels for the same reason. As the text size increases, the content needs to adjust accordingly. Use relative units such as
rem
and amax-width
value. - You should not apply styles directly to the HTML element. Add a class and style it through the class. For example, you set a width of 30px on the
img
element. If there are other images on the page, they will all be 30px wide. - Overall, you need much better structure in your HTML and CSS. Specifically, the CSS is hard to navigate and understand. Work from top to bottom, left to right. Organize your HTML with the correct semantic elements.
There are still other things I could comment on, but this is already quite a long list. Let me know if anything is unclear. Once you've refactored your solution, I can take another look and point out additional areas for improvement.
Cheers, and happy coding!
Marked as helpful2 - @sinasami000Submitted 3 months agoWhat are you most proud of, and what would you do differently next time?
this is the first time I did something like this. I hope you like it
@gmagnenatPosted 3 months agoHi congrats on completing the challenge !
here are a few points you can check up to help you improve your solution.
HTML
- You are missing a <main> landmark. It's needed to indicate the main content of the page.
- Don't mention "image" in alt attribute. Check out this ressource to help you write better alt : How to write good alt text for screen readers
- You shouldn't skip heading order to keep a logical flow in your web page. You can have different styles for h2.
- You shouldn't use the <b> tag. Use <strong>, it has a semantic meaning here.
- In your <table> element you need to use also <th> to indicate the heading of the line.
CSS
- You should put your style in a separated stylesheet when it's more then a few lines of code.
- You need to add a Modern CSS Reset at the top of your stylesheet in all your projects
- Don't use pixel for anything related to fonts. Why font-size must NEVER be in pixels
- You shouldn't style the html element directly unless its a reset. Add class to your element to style them. It will help you have a more maintainable solution if you want to change the html tag tomorrow you don't have to update the stylesheet.
- Use relative units when using max-width. It will let your element containing text scale with the size of the text. The user can change the default font-size of the browser so you need to make sure your solution can adapt.
- You should relative unit in your media query. It is also needed for when the default font-size is changed by the user. The layout need to adapt with the user setting.
I hope you find something useful in this list. Let me know if one of the point isn't clear enough.
Happy coding !
0