order summary

Community feedback

• 

  T

  Grace•31,090

  @grace-snowPosted almost 2 years ago

  Hello

  This looks a little broken on my phone. Hopefully these suggestions will help with that and with the overall code quality

  • the background image should be just that. Make it a background image on the body. It does not belong in the html
  • main image icon is not appropriate alt text for the top card image. If you consider this as a meaningful image it must describe what the image actually looks like. If you consider the image to be decorative, alt should be left intentionally blank.
  • Additionally, alt should never have words like "image" or "picture" in them as they already have an explicit image role (they are an image element)
  • similarly the music icon does not have the correct value in its alt attribute. That image is definitely decorative so alt should be blank
  • you are missing at least one heading element in this card
  • text should never be in divs or spans alone. Always use a meaningful element (headings, paragraph etc)
  • a general point that seems small but is an important habit to build - Indent your code consistently. Your code editor can even do this formatting automatically for you. This makes it much more readable and easier to find bugs, especially in html, but also in css and js
  • In every challenge think through which interactive elements you should use each time. On this challenge there are 3: change, proceed and cancel. The elements you've chosen may be fine or they may not, it all depends on what you think would happen on click of each of them. If clicking would navigate the user to another page, that should be an anchor element; If clicking would perform an action like toggle content or submit a form, that should be a button element.
  • Remove all brs in this. You will rarely have to use that element. It should never be used to create space between elements (that's what margin is for)
  • When using button elements it's good practice to always include a type. This can just help avoid bugs later on in bigher projects

  That's HTML feedback done, now onto CSS...

  • The body should never have it's width limited. Think of it like your piece of paper that everything else goes on - it should always be able to be as big as the users screen
  • Never use width: 100vw anywhere. Viewport units don't account for scrollbars or device notches so what you're actually doing there is creating horizontal scroll in many instances because you are telling the browser to make main "100% of the screen width plus the scrollbar width"
  • don't limit main height to 100vh. Use min-height instead. Because you have used height there the card contents are getting cropped at the top for me on mobile. Min height is much better to use so it can grow beyond the screen height if it needs to
  • main should definitely not be position absolute!
  • The card should not have a width, only a max-width. Similar principle to before - width is fixed so doesn't allow the component to be responsive. Max width says "let this shrink if it needs to" (like on a smaller screen)
  • the card (and actually no element containing text) should have a fixed height. This is really important. You never want to limit height like that. Let the browser decide how tall the card needs to be based off the content within it + paddings + margin on internal elements. The browser will know what adjustments are needed if the user changes their text size or if a content editor changes the amount of text in the card. Don't micromanage, let the browser do it's job
  • Font size must never ever be in px. Again, users change font sizes sometimes and setting px font sizze removes that power from them, potentially making your solution completely inaccessible
  • the main image does not need a width or height. Every project should include a basic modern css reset at the start of the stylesheet. Amongst many other things, this sets img tags to display block and max-width or width 100 %. That's all the main image needs, and optionally an object fit property
  • plan does not need a width or height either. It would fill the available space by default and be as tall as it needs to be based off content
  • the music icon (and icons in general) are one of the few times it IS recommended to have an explicit width and height. They are fixed
  • buttons should not have a width or height (unless width 100%)
  • consider making reusable classes for interactive elements. This is only a single component challenge but code it as if it's not - all links and buttons might have cursor pointer by default; maybe you have a link class for general styling then a modifier class to change the color of those links when needed; maybe you have a button class...
  • next time make the mobile styles your base, and only override what you need to for larger screens in a min width media query.
  • Don't repeat any styles in media queries. The base styles would still apply
  • The media query on this can be waaaay shorter. I think the only things that change (once the above suggestions are implemented like removing all those fixed widths) are some spacing sizes for things like padding.
  • make sure the card has a little margin around it so it can't touch screen edges
  • it is slightly more performant to link fonts in html head instead of importing in css
  • on mobile at the moment the attribution is sitting over to the side of the card, so I have a load of horizontal scroll. Possibly caused by a display flex at its default direction of row instead of column (or sometimes caused by position absolute)

  Good luck

  Marked as helpful

  0