Skip to content
OverviewSolutions
7
Comments
2100
T
Grace
@grace-snow

All comments

  • Grace•31,330
    @grace-snowPosted 8 days ago

    Oh no I just wrote loads of feedback then accidentally deleted it 😭 😆

    Oh well ill have to summarise quickly, there wasn't much:

    • you can't aria-labelledby an article by itself! That makes no sense and would lead to a bad experience for screen readers. If you really want to label these it should be pointing to the person's name.
    • the quote paragraphs should be in a blockquote element.
    • I'm not sure why you're adding tailwinds reset and then another reset.

    I can't really comment on css more than that as I'm only looking on mobile.

    Make sure you keep learning html even as you progress in the js learning path.

    Marked as helpful
    1
  • Lisa Allen•550
    @CrypticMangoSubmitted 11 days ago
    What specific areas of your project would you like help with?

    Any tips will be helpful! Thanks for taking the time to look at my work :D

    Recipe Page Vanilla HTML and CSS

    2
    Grace•31,330
    @grace-snowPosted 10 days ago

    This is overflowing on the sides when I view on mobile because you’ve used explicit widths.

    I’m a little surprised to see the code in this actually as it looks like you’ve done a lot of challenges already and can’t have had good quality feedback. I’ll try to list out a load of things and hopefully they can help you go back and refactor this and others.

    • to achieve the layout easily in this I’d expect all text content to be in one div, a sibling to the img. This allows you to make the padding changes in one go from mobile to desktop, applying padding to the text div in mobile only. This removes all the need for strange side paddings or margins on child elements which are unnecessary.
    • aria-label does not belong on an img and must be removed. The alt needs to give a proper description of the image, conveying the same information as the image does. Craig Abbott has written a great blog article about how and when to write alt text if you want to look that up.
    • headings should go in hierarchical order. It’s about structure, not sizes in the design. That means preparation should be a h2.
    • the spans with title class should be strong tags because they are there for emphasis.
    • in the table it is essential to use header and data cells appropriately. They cannot all be header cells.
    • the header cells that should be there should also have the scope attribute set to “row” to make clear they are row headers and not column headers.
    • on the GitHub link at the end the aria-label needs to be placed on the link itself and not on the icon. Meaningless elements cannot have aria-labels, that can only go on an element that has an allowed role.
    • get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell has one that would work well on this and most projects.
    • don’t style the html. The background colour should be on the body. And make sure it’s the same background colour that was in the design.
    • font sizes should always be in rem not px. https://fedmentor.dev/posts/font-size-px/
    • it’s very unusual to place padding on text nodes like headings. Make sure you understand the difference between padding and margin (there’s a post about that on the same site just linked)
    • the component should not have a width, only a max width. And this should be in rem so that the layout scales well for all users including those with a different font size setting. It’s important to understand why you should be using max-width and not width here — so that the component is able to shrink narrower when it needs to like when there isn’t enough space.
    • similarly the img must not have a width. It doesn’t need a rem max width either. It only needs what is already included in any modern css reset: width 100% and display block.
    • the table should be like the image and only have a max-width 100%. The table will also need collapsed borders.
    • really this and most components should be styled mobile first. It wouldn’t be much work to change that even at this stage.
    • I recommend using rem or em to define media queries, not pixels. Again this is to help those who change their default text size, ensuring the layout switches for them at an appropriate time. (There’s an article about media queries on the site too)
    • very little needs to change between the mobile and desktop view in this challenge. If you go through the above changes particularly removing all the side paddings and margins I’d expect there only to be a couple of lines needed in the media query to change the max width of the component and where the padding is applied.

    Good luck.

    Marked as helpful
    3
  • Grace•31,330
    @grace-snowPosted 10 days ago

    This doesn’t really look close enough to the design yet. Try and get the font styles and all styling much closer.

    In the html I notice some important changes are needed:

    • headings must go in hierarchical order and not skip levels. This is a valuable accessibility consideration.
    • bold text for emphasis is done with strong not spans.
    • the table must use header cells for header content.
    • those header cells should also have a scope attribute to make clear they are row headers not column headers.

    And in css

    • get into the habit of including a full modern css reset at the start of the styles in every project you do. Andy Bell or Josh Comeau both have good ones you could use.
    • place font-face declarations before where you use them in the css.
    • make sure you include fallback fonts
    • I’m not sure you mean to be using rem for so many inline paddings and margins. Make sure you’re only using that for properties that you’d want to scale with the user's font size.
    • similarly, you’re using em units a lot which is unexpected and could lead to unwanted results. Only use em on specific properties where you want it to scale with the current element’s font size. For example, button padding is commonly done with em or vertical margins above headings and paragraphs. It’s usually only used quite rarely.
    • the max width on the main component should definitely be in rem. That’s a width that needs to scale with the users font size.
    • Personally I would style the white box as a component inside the main landmark instead of making it be the main landmark. This is because on a real site there may be pages where the main wouldn’t have a limited width and style like this.
    • there needs to be a media query in this challenge. Currently it looks broken viewed on my mobile. Everything is aligned over to the side and the table lines overflow the main landmark. Make sure you check all screen sizes as well as changing browser font size and zooming in/out to cover everything.
    • the line heights look too large on the main heading and there are inconsistencies in font size.
    Marked as helpful
    2
  • Alexander Roan•170
    @dearestalexanderSubmitted 15 days ago
    What are you most proud of, and what would you do differently next time?

    Note there is a link in the attribution at the bottom to switch between the yamanote design and the testimonial design.

    Thanks to the really good grid training materials I didn't have too many problems with this exercise. In addition to grid for the macro layout I found grid useful to align the small headshot images with the name and role text.

    In general I find grid is a little more predictable than flex for quick use and is more concise from a CSS point of view. So I can imagine I will use it more.

    For my alternate design I had a bit of fun making the background image in figma. Starting to think about background images was a nice process from a design standpoint for me and I think there's a lot of fun to be had here, although I appreciate they are just decorative and not very functional.

    What challenges did you encounter, and how did you overcome them?

    Nothing major, I learnt during this exercise you can't easily change opacity of background images so you need to either adjust your .svg or use some tricks.

    What specific areas of your project would you like help with?

    Nothing in particular, but always open to feedback on my design or anything you think in my CSS is not optimal.

    Testimonial grid with alternative 'Yamanote Line' design

    2
    Grace•31,330
    @grace-snowPosted 15 days ago

    There are html problems in this that need fixing. They should be quite easy to fix though.

    • The h1 heading level is for the main heading on a page. You shouldn't ever have more than one h1. I recommend you add a h1 above the grid for the whole page as you appear to have made this as a full page design rather than a component/section like the original design. Or if this would just be a section on a page, then you can leave out the h1 in this. Either way, change all those article headings to h2s.
    • there is invalid html on the img element. Width and height attributes in html must not have px inside their values. I don't think you need that attribute at all though as it's only meant to be used if you are also using the height attribute. Together these attributes tell the browser what aspect ratio the image should be, and improve performance by saving the space for the image while it's still loading.
    • I notice you've placed an image on its own inside a link. That means it's alt text must state the destination of the link. It currently says "promotional graphic for the suica travel card featuring it's mascot penguin" Which doesn't communicate the link destination. This would be an accessibility failure under WCAG criterion 2.4.4 Link Purpose
    • it also looks like that image contains text which can be another accessibility problem because people can't change the text styling to make it more readable. That image may be exempt from this if it's a logo but I can't tell, as I'm not familiar with it. If it isn't a logo, there needs to be text along with that image.
    • remember all content should be contained within landmarks. This should all be inside a main landmark, and the attribution in a footer.

    I had a quick look at the css as well

    • width: 100vw shouldn't be on the body or used anywhere really! All that cam do is cause overflow — unwanted horizontal scrollbars to appear for some users — because the vw unit doesn't account for scrollbars when they are present. The body is already full width so you don't even need that line.
    • don't use explicit grid column widths, especially not in px. I can see this challenge will be broken when I view it on various screen sizes, zoom levels or font sizes. Use 1 fr instead.
    • I doubt you want to be using rem for the padding on these grid items. Remember that will scale with users font size so could lead to very narrow content boxes!
    • I expect the content (the grid) to have a max width in rem to limit its width.
    • the mobile styles should definitely be the default in this design.
    • I recommend against using complex css selectors that target elements. Thia is because it will break the styles when html has to change, and creates css that is more specific and much harder to manage on larger projects. Stick to single class selectors as much as possible. Keep css specificity as low as possible.
    • the items shouldn't have widths in percentage. I can't understand why that's been added. The grid can have max width in rem and an optional width in percentage.
    Marked as helpful
    1
  • Lokesh•120
    @Lokesh8055Submitted 17 days ago
    What are you most proud of, and what would you do differently next time?

    These 2 projects has really helped me to learn the rules that I need to follow for creating a project in frontend Mentor.

    What challenges did you encounter, and how did you overcome them?

    Encountered challenge with using boxShadow for the card.

    What specific areas of your project would you like help with?

    Please go through my project and let me know on the areas that I can improve.

    Blog Preview Card Solution using HTML and CSS

    1
    Grace•31,330
    @grace-snowPosted 16 days ago

    The same feedback I've just left on your previous solution applies here so I won't repeat.

    There's one really important extra consideration you've missed in this challenge though: the link. The design shows this card is meant to be clickable, but you've not included the essential interactive element, instead adding hover styles to non-interactive elements. This makes the whole component non-functional (I.e. Pointless! When it's purpose is to link to a blog).

    Once the link has been added inside the heading, you'll need to make the whole card clickable. A common way to do that is by making the card position relative and adding a pseudo element to the link that covers the card with position absolute. Now tye clickable area of the link will cover the card.

    The only other problem I see is the max-width 25% on the tag/category. You need to remove that. It will cause overflow for some users. Let the width of the category/tag be defined by its content and padding only. There's no need for a width or max-width.

    Lastly, there's no need for a media query in this challenge. You can remove it all. Leave the font sizes as the default and don't worry about changing anything.

    0
  • Lokesh•120
    @Lokesh8055Submitted 30 days ago
    What are you most proud of, and what would you do differently next time?
    • Proud of Learning about frontend mentor rules for creating a project.
    What challenges did you encounter, and how did you overcome them?
    • Encountered challenge with Readme file, it's a very good learning for me to create proper documentation
    What specific areas of your project would you like help with?
    • Please go through my project and let me know on the areas that I can improve.

    Qr code component challenge using HTML and CSS

    2
    Grace•31,330
    @grace-snowPosted 16 days ago

    Great feedback above and we'll done for applying changes so quickly.

    I've got a few more recommendations:

    • get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
    • Look up how and when to write alt text on images. There are some great posts about this in the resources channel on discord. Remember, alt is human-readable content not code. This image is really important, so needs a proper description. It needs to say what the image is (QR code) and where it goes to in this case (to FrontendMentor.io).
    • when building single components like this you still need to consider the context of where that component would be used in a real site. This card would never be used to serve the main heading on a page, so you know it shouldn't have a h1. Use a lower importance heading level like h2 instead.
    • it's better for performance to link fonts in the html head instead of css imports.
    • make sure the component can't hit screen edges by giving a wrapping element a little padding on all sides (eg body or main in this)

    Note the deployed link seems to be broken above so I can't preview the finished version at all.

    Marked as helpful
    0
  • Dave Quah•660
    @MilleusSubmitted 25 days ago
    What are you most proud of, and what would you do differently next time?

    I'm proud of getting back into Frontend Mentor challenges as I've been away for many months. I aim to dedicate more time into learning outside of work.

    What challenges did you encounter, and how did you overcome them?

    The challenge I've encountered is on how to use more meaningful semantics for the element.

    Referencing the MDN website, I've learnt that I could use the datetime attribute of a element to describe the publication date and time of an element.

    Blog preview card using variable fonts

    1
    Grace•31,330
    @grace-snowPosted 24 days ago

    You need to switch that aria-desciribedby to aria-labelledby. Using description means the blog heading is read out after everything repeatedly.

    The other small issues I noted

    • this card would never be used to serve the main heading on a page. So you know it shouldn't have a h1. Use a lower importance heading level like h2 instead.
    • you don't need aria hidden on decorative images. You have correctly left the alt empty so it's already been hidden in the right way.
    • the description on the author image is only repeating the text that's next to it, leading to unnecessary screen reader announcement. It's bringing no value so treat that image as decorative too with an empty alt.
    • font size should always be in rem not px. See https://fedmentor.dev/posts/font-size-px/
    • I strongly recommend you stop using all these complex direct child css selectors. On a big project that will create css with high specifity that's very difficult to maintain. Keep css specificity as low as possible. Use single class selectors.
    • note when links open in a new tab like in that attribution link it's best practice to let screen reader users know in advance. E.g. include some visually hidden text in a span that says "(opens in a new tab)".
    • the design indicates that this whole card should be clickable. See if you can work out how to do that in a simple way without moving the link (it's in the correct place in the html at the moment - I could say exactly how but you may learn better trying to sort this one on your own first, maybe you just forgot to implement etc)
    Marked as helpful
    0
  • Grace•31,330
    @grace-snowPosted 24 days ago

    I'm afraid there are some small but severe accessibility problems in this solution that need fixing.

    1. Every input must be labelled.
    2. You must not use the auto focus attribute. Let people have full control over where focus goes. Currently it's like screen reader and keyboard users will get hijacked.
    3. All of these inputs must have autocomplete attributes set to appropriate values.
    4. The submit button should be aria-desciribedby the unique ID of the terms and conditions paragraph.
    5. The errors need to be announced by screen readers when they appear and should be programmatically linked to their inputs. As you're already conditionally rendering them, the easiest way to address all this is to wrap each error in an extra element like a div that's got a unique ID and an aria-live attribute. Then add aria-desciribedby to the input referencing the id of the error wrapper. This will mean that the errors are announced when they first appear, and on focus of the input (read out after the input label).
    6. You need to remove the max-width 100vw and overflow-x hidden from the html and body. Firstly, these are unnecessary. The html doesn't need styling and should be left alone. The body is already full width by default. But adding the max width can make it impossible for people to read content if they have a large text size or use zoom and view on a smaller screen. People need to be allowed to scroll sideways if overflow ever happens.
    7. Font size needs to be in rem not px. For more info see https://fedmentor.dev/posts/font-size-px/

    And general observation/recommendation:

    • use the fonts and sizes as defined in the style guide (except with font size converted to rem before use)
    • try and get this much closer to the design in terms of these things. Yours is currently much smaller for example.
    • don't forget to adjust line height. Usually that will be unitless like 1. Etc.
    • you seem to be using percentages for various properties and setting widths unnecessarily in lots of places. To limit width, use max-width in rem. Remember padding — that's usually in px on the sides as that's not a property you'd want to scale with the users device width or font size. Using percentages will often end up with confusing or unpredictable results, especially for properties like margin. There is a post about padding vs margin and another about media queries and responsiveness on my site shared above and those articles may help you overall.
    • The other recommendation is to watch Andy Bell's talk "Be the browser's mentor not it's micromanager" as this may help you with overall layout approach.
    0
  • Grace•31,330
    @grace-snowPosted 24 days ago

    Hi,

    This looks good but there are a few issues that should be addressed.

    • the 3 stats need to be an unordered list with 3 list items. Each number and/or word in those can be in spans set to display block to make them stack vertically on each line.
    • the card must not have a height. That's very potentially dangerous for overflow bugs. You can't control the height — you have no control over the end font family, language, font size or line height the end user may have, and all of those could change where lines break and how tall the component needs to be. It's the browsers job to decide the height based off the content, plus any padding, margin or gaps used inside it. You could use min-height but even that is unnecessary.
    • the text half of the card should have padding on all sides. The low level child elements like headings or paragraphs shouldn't need percentage widths. Don't micro-manage. All the component needs for width is a max-width in rem (different on mobile default and larger screens in the media query).
    • it's unnecessary to load the css reset as a whole other file. That impacts performance. Instead, just place those css declarations at the start of the main styles.
    0
  • justine1607•140
    @justine1607Submitted 26 days ago
    What are you most proud of, and what would you do differently next time?

    I am proud that i can increment, decrement and remove item form the cart using vanilla js.

    What challenges did you encounter, and how did you overcome them?

    the challenge that i encountered were a lot just let me state one. -honestly it was so difficult because i still lack of knowledge on javascript especially on the removing the item, it took me 2 days before i gets on how the logic will work, but then, because of my enthusiast i have overcome this difficulties by just trying and trying until the code works but also with the understanding on how they work.

    interactive add to cart

    2
    Grace•31,330
    @grace-snowPosted 25 days ago

    I think you're jumping ahead with this site. Particularly in terms of accessible HTML and JS. This is inaccessible to people who use a keyboard or screen reader at the moment, which is a baseline requirement.

    Also the performance is suffering because of the way you're loading the images.

    For example you need to

    • update all image alts especially iconography
    • use interactive elements for all controls
    • ensure those interactive elements always have an accessible name that says what they do (through img alt, sr-only text or aria-label)
    • the cart button wouldnt usually open a modal on click, there would be a form submit. But any button that opens a modal dialog would have aria-haspopup="dialog".
    • use the native html dialog element for the confirm. Follow the docs for that element and it should be accessible by default. Without that, you'll need to do loads of extra work.
    • use the picture element for the product images.
    • give product images an aspect-ratio in css to help loading performance, or a width and height attribute in the markup.
    • probably give them loading="lazy" (although I'm not sure if that works when loading the markup with js... Worth a try! Watching the network tab as you scroll down on a short viewport would show if lazy loading is working).
    • as a general rule, use JS to toggle classes and attributes instead of lots of inline styling. This leads to a much more manageable project. For example, the JS can toggle the hidden attribute instead of display none, OR toggle classes like "is-visible".
    • this may need some sr-only text elements and/or aria-live regions to ensure that screen readers are notified of content changes.
    0
  • justine1607•140
    @justine1607Submitted 26 days ago
    What are you most proud of, and what would you do differently next time?

    I am proud that i can increment, decrement and remove item form the cart using vanilla js.

    What challenges did you encounter, and how did you overcome them?

    the challenge that i encountered were a lot just let me state one. -honestly it was so difficult because i still lack of knowledge on javascript especially on the removing the item, it took me 2 days before i gets on how the logic will work, but then, because of my enthusiast i have overcome this difficulties by just trying and trying until the code works but also with the understanding on how they work.

    interactive add to cart

    2
    Grace•31,330
    @grace-snowPosted 25 days ago

    General important point about repositories - node modules must never be pushed to the remote repo!

    To remove

    1. locally delete node modules
    2. Add it to the gitignore (it should have been in there at the start TBH)
    3. Commit and push that change.
    4. Re install locally (npm install)
    5. If working on a repo with a build step you'd give the host (eg Netlify or Vercel) that build command so it can build your site before any deploy. (If all you're doing like here is building some sass on a practice project you can choose to build the css locally before pushing to the remote repo — it's not strictly necessary for the host to build the css for you although that is recommended).
    0
  • Grace•31,330
    @grace-snowPosted 26 days ago

    I've already left a lot of feedback on your blog card solution so won't repeat things here.

    But this challenge also needs changes. The whole questions should be inside the buttons, not just the icons.

    This is an accordion challenge, which means it is a set of disclosures. These have very specific accessibility requirements that you must follow. See the article here for essential changes needed https://fedmentor.dev/posts/disclosure-ui/

    Once you've fixed the markup you should change the JS too. Loop over all the buttons and add event listeners to run a function. That function should change the aria expanded attribute and optionally a class or data attribute to manage the visibility of the answer. The same single function should run on click of any of the buttons. Remove all onclicks in the html and rely on the one short js function instead.

    Marked as helpful
    0