Column Preview Component Using CSS3 Flexbox

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello @osmannurierdogan ,

  I have some suggestions regarding your solution:

  • There should be two landmark components as children of the body element - a main (which will be the component ) , a footer landmark for the( attribution).

  • For any decorative images, each img tag should have emptyalt=""(as you did )and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only.

  • swap the buttons for anchor tags. Clicking those "learn more" buttons would trigger navigation not do an action so button elements would not be right. And for future, it is essential if you include a button in a form element without specifying it's just a regular button, it defaults to a submit button., though, so it's a good idea to make a habit of specifying the type

  • you can add a <h1> with class="sr-only" (Hidden visually, but present for assistive tech). Then use <h2> instead of`` <h1>```.

  • don't capitalise in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalised text as they will often read them letter by letter thinking they are acronyms.

  
  .sr-only {
  	border: 0 !important;
  	clip: rect(1px, 1px, 1px, 1px) !important;
  	-webkit-clip-path: inset(50%) !important;
  		clip-path: inset(50%) !important; 
  	height: 1px !important;
  	margin: -1px !important;
  	overflow: hidden !important;
  	padding: 0 !important;
  	position: absolute !important;
  	width: 1px !important;
  	white-space: nowrap !important;            
  }
  
  

  • The links learn more moves up on the hover , it's not normal , you can solve it by setting border: 2px solid transparent ;(instead of border: none; ) and the on hover will border: 2px solid`var(--light-gray-button-background-heading-color) ;(It's better not to use px`` for the boder-width )

  • I recommend to use em and rem units .Both em and rem are flexible, Using px won't allow the user to control the font size based on their needs.

  • using widths in percentage. Not a great idea as you're losing control of the layout

  • You can use flexbox properties and min-height: 100vh; to the body to center the component on the middle of the page.(no need for margins ).

  • width: 375px;an explicit width is not a good way . Remove the width from the main component and change it to max width instead. That will let it shrink a little when it needs to.

  -- what about the mobile solution ? it t should be a flex column.

  Overall , your solution is good . Hopefully this feedback helps

  0

  ---

  

  Osman Nuri ERDOĞAN•330

  @osmannurierdoganPosted almost 3 years ago

  @PhoenixDev22 Thanks a lot, your comments are very important for newbies like me :) I will pay attention on them and update my solution soon hopefully . :D

  0