Flexbox, Grid, Absolute Positioning, Relative Positioning

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted over 3 years ago

  Hey, great work on this one. The layout in desktop is kinda huge because of the height of the body, the mobile state is good just that image, but @Skyz03 already mentioned on how to fix that, works for point 450px downwards but not further from that.

  Other suggestions would be that:

  1. On the desktop layout, instead of using min-height: 1080px; use min-height: 100vh. This will make sure that your layout, the body rather have the whole viewport. Then just add padding to the top and bottom of the body to prevent the element touching the ceiling and flooring of the viewport.

  2. Adding a cursor: pointer to the accordion buttons will be awesome, since it is interactive element, it will make it more natural.

  3. On the mobile state, I think your layout needs more width. Because if you look at it, it is somehow clamped right, the padding as well on the accordion is too much I think.

  4. Lastly, it will be better if you put the javascript inside their own module and not using inline javascript. This is a good practice and this is really a must.

  But still, good job on this^

  1

  ---

  

  Taariq Mamun•50

  @taariqamozillaPosted over 3 years ago

  @pikamart Thank you very much! I've implemented most of these and actually forgot to seperate the javascript... It took me a while but i got there in the end with the alignment of the image, I completely forgot the parent was was display flex and was trying to do it with positioning instead of just "align: center"

  0

  

  Taariq Mamun•50

  @taariqamozillaPosted over 3 years ago

  @taariqamozilla Actually i still have some issues with the desktop view, if you could help me with the box that floats up when all the accordions are open and also the image to the left cuts off past a certain point instead of extending because of my overflow property.

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 3 years ago

  @taariqamozilla Oh sure, I can help you with that^^

  To prevent that from happening, what we can do is that, we make the right container, the container that holds the accordion, we make it have a maximum-height. Then we are adding a overflow: scroll on it, this way, the container will not be resized because we set a max-height on it, and we can just scroll the accordions. But this will create a scrollbar right, but we can remove that and still have the functionality.

  Here's what you will add:

  First, add this to the card-accordion selector

      max-height: 31.875rem;  #this is equivalent to the 510px you set as the min-height of the whole container
      overflow-y: scroll;  # we use this so that we could scroll on the overflowed accordions
  

  Second, we need to remove the scrollbar, add these

  .card-accordion::-webkit-scrollbar {  
    display: none;  # this will remove the scrollbar but keeps its functionality
  }
  
  /* Hide scrollbar for IE, Edge and Firefox */
  .card-accordion{
    -ms-overflow-style: none;  /* IE and Edge */
    scrollbar-width: none;  /* Firefox */
  }
  

  After adding those, your container will now be okay and no more resizing will happen. Though you might need to adjust the max-height on the mobile state, if you were to implement that again.

  If you need more help, just drop it here okay and i'll help you^^

  0
• 

  P

  Skyz Walker•1,035

  @Skyz03Posted over 3 years ago

  Hi Taariq, I had a quick look at your code via inspector and the error i found was.. in your mobile-hero class if you remove left : 28% if work perfectly align in the center... I think you can implement this via @media queries for that size as in tab or desktop version that is needed.

  If you find this feedback useful please up vote this comment nor let me know more by replying back in the comment .

  Cheers have a great day ✌.

  1

  ---

  

  Taariq Mamun•50

  @taariqamozillaPosted over 3 years ago

  @Skyz03 Thank you! I've removed the 28% and aligned it in the center, I didn't realise that I could use flex and it completely skipped my mind.

  0