search

layer5io / layer5

Layer5, expect more from your infrastructure
https://layer5.io
Apache License 2.0
817 stars 1.08k forks source link

Replaced carousel library #1236

Closed OmkarPh closed 3 years ago

OmkarPh commented 3 years ago

Signed-off-by: OmkarPh omkarphansopkar@gmail.com

Description

This PR fixes #

Notes for Reviewers

Signed commits

netlify[bot] commented 3 years ago

Deploy preview for layer5ng ready!

Built with commit 44e49ac05d952261db2d7b505b4433ab8492adc1

https://deploy-preview-1236--layer5ng.netlify.app

OmkarPh commented 3 years ago

Hi @Jashpatel1 I realized that react-slick is already in use by component slick.slider.js in reusecore

In the preview, I've shown carousel using both react slick and swiper JS(2nd one)

React slick ( 1st one ) looks better to me.

I tried shifting arrows in Swiper JS outward but it has a overflow hidden property (necessary) which forces to conjest arrows.

Jashpatel1 commented 3 years ago

Hi @Jashpatel1 I realized that react-slick is already in use by component slick.slider.js in reusecore

In the preview, I've shown carousel using both react slick and swiper JS(2nd one)

React slick ( 1st one ) looks better to me.

I tried shifting arrows in Swiper JS outward but it has a overflow hidden property (necessary) which forces to conjest arrows.

@leecalcote @Nikhil-Ladha @Tanuj22 ^^

Tanuj22 commented 3 years ago

I am not seeing any difference in both of them :sweat_smile:. If you say so we can go with the 1st one. Though the arrows are not looking nice currently. Also, the mobile view needs to be taken care of.

OmkarPh commented 3 years ago

I prefer 1st one not because of styling, it can be manipulated as we want ( I'll try to change em and make minimal )

But in 2nd one the arrows and corner cards are so close and pose problems in wider corner cards

Nikhil-Ladha commented 3 years ago

Okay, so I have a different opinion here. In mobile, the second is better because of the feel/animation while swiping the cards. But again in desktop view, the animation is better for 1st one, not sure if that can be changed :thinking:

But in 2nd one the arrows and corner cards are so close and pose problems in wider corner cards

I think there should be a way to manipulate that as well, if not directly then you can refer to the class that is being used by inspecting and then add CSS for it. That will work.

OmkarPh commented 3 years ago

But in 2nd one the arrows and corner cards are so close and pose problems in wider corner cards

There were 2 ways to solve this:

  1. Provide a margin for arrows Coz of this, arrows eventually become invisible partially because of hidden overflow.

  2. Reduce size of cards wrapper This cards wrapper width is way more than screen width and overflowed by default in component so modifying its width is useless.

I tried many other things but unsuccessful ☹️

leecalcote commented 3 years ago

I agree that the 1st one has a smoother, more desirable swiping animation and that we should try to retain this animation whether the 1st or 2nd one is used.

OmkarPh commented 3 years ago

Should i make the arrow of first one like second one? Plain blue or layer5's green

Nikhil-Ladha commented 3 years ago

Layer5 green

OmkarPh commented 3 years ago

Can you tell me how to view this on mobile within same network ( not available on PC's ip now ) ? I usually test with dev tools but for proper swipe feeling, wanted to test on my mobile

OmkarPh commented 3 years ago

Note - I've used window object for toggling arrows, dots and no. of cards to show, so reload after changing resolution in dev tools :)

leecalcote commented 3 years ago

Regarding the first implementation, note that the righthand arrow flickers and half hides itself on hover.

Nikhil-Ladha commented 3 years ago

I think we would want to use the colors in the following way? Screenshot from 2020-12-10 11-39-41

OmkarPh commented 3 years ago

I think we would want to use the colors in the following way?

@Tanuj22 @Jashpatel1 @leecalcote ^^

Tanuj22 commented 3 years ago

I think we would want to use the colors in the following way?

@Tanuj22 @Jashpatel1 @leecalcote ^^

I agree with @Nikhil-Ladha . Rest looks great!. Do remember to take care of the mobile view :)

OmkarPh commented 3 years ago

Is current disabled color ok for arrows? image Also, these dots for mobile: image

Nikhil-Ladha commented 3 years ago

Not sure if we want to keep the dots or not, is there any way to remove them? Also, hide the arrow if it's disabled.

OmkarPh commented 3 years ago

Not sure if we want to keep the dots or not, is there any way to remove them? Also, hide the arrow if it's disabled.

In mobile view, user would generally prefer swiping so I disabled arrows there. Without dots, users might not understand if this is a carousel at all and they can swipe too. So kept dots only for mobile.

Nikhil-Ladha commented 3 years ago

In mobile view, user would generally prefer swiping so I disabled arrows there. Without dots, users might not understand if this is a carousel at all and they can swipe too. So kept dots only for mobile.

For mobile view, we might keep that. But not for desktop view. Also, please check the alignment of carousel in mobile view. It's not center aligned.

Tanuj22 commented 3 years ago

The mobile view is left ...

OmkarPh commented 3 years ago

The mobile view is left ...

I fixed that, the cards are centred and take up maximum possible space horizontally now.

Tanuj22 commented 3 years ago

The mobile view is left ...

I fixed that, the cards are centred and take up maximum possible space horizontally now.

Why I am seeing this :thinking: Screenshot from 2020-12-11 14-20-45

Tanuj22 commented 3 years ago

Nevermind it was due to directly switching to mobile from desktop view without reloading :sweat_smile:

OmkarPh commented 3 years ago

Also this tablet view is taken care of: Screenshot_2020-12-11-14-21-28-23

OmkarPh commented 3 years ago

Nevermind it was due to directly switching to mobile from desktop view without reloading

Ya that is a problem due to window obj😅 Do we have anything else that can be used to get innerwidth in gatsby?

Tanuj22 commented 3 years ago

Nevermind it was due to directly switching to mobile from desktop view without reloading

Ya that is a problem due to window obj Do we have anything else that can be used to get innerwidth in gatsby?

Not at the top of my head. Will let you know if I find anything

OmkarPh commented 3 years ago

I found this, https://reedbarger.com/how-to-create-a-usewindowsize-react-hook/