Testimonial grid with alternative 'Yamanote Line' design

Community feedback

• 

  T

  Grace•31,790

  @grace-snowPosted about 1 month 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

  ---

  

  P

  Alexander Roan•170

  @dearestalexanderPosted about 1 month ago

  @grace-snow

  Hello and thanks for all the detailed feedback. Very much appreciated. Helping with the learning a lot!

  I am still unsure how best to approach width on things. Based on your feedback I tried setting a minimum for the desktop grid, but let it get bigger (width: max(69.375rem, 77%) with four columns 1fr 1fr 1fr 1fr.

  Then I put two breakpoints where I reduce columns down to '1fr 1fr' then '1fr' where I also specify where to position some of the items.

  I also gave the items that don't span columns a min-width: 15.9375rem to stop them becoming very thin before the screen width hits the breakpoints.

  I did have a try of doing it without breakpoints, but grid just went from four columns to one very wide column (using repeat and auto-fill).

  Would you say the above is a good approach?

  The rest I understood. I'll need to adjust my approach to CSS naming to make it less dependent on the HTML structure.

  Thanks!

  0

  

  T

  Grace•31,790

  @grace-snowPosted about 1 month ago

  @dearestalexander Grid children shouldn't have a min-width or width. Only the grid template should be defining that.

  I'm not sure why you're trying to use strange percentage widths like 77% either?

  All the grid should need is something like max-width: 70rem, nothing complicated. You can set different rem values for this in each breakpoint if you wanted to but it may not be necessary.

  If grid items are getting far too narrow / breaking when close to breakpoints that hints that the breakpoints may be set at the wrong places (but I suspect the min and max widths issue I've just explained may fix it)

  Marked as helpful

  0

  

  P

  Alexander Roan•170

  @dearestalexanderPosted about 1 month ago

  @grace-snow Hi Grace, no worries, I posted that before we talked on discord and I've been doing a little work on it. Working much better now. I understood your advice on width :)

  Will be updating it sometime later today when I have a lot of the issues fixed :)

  1
• 

  Yacoub AlDweik•1,830

  @YacoubDweikPosted about 1 month ago

  Hey!

  Good job man just one small thing:

  Don't give body { width: 100vw } instead it has a default value of 100%.

  I was resizing the window using the dev tools in Chrome and I kept getting a horizontal scroll bar, like in all cases there's no need for it the body will take 100% of the screen in all cases.

  Keep it up, I really liked the idea of having 2 designs!

  0

  ---

  

  P

  Alexander Roan•170

  @dearestalexanderPosted about 1 month ago

  @YacoubDweik Thanks! fixed that part now I think and loads of others haha.

  0