layer5io / layer5

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

[Integrations] Convert stack of category filters to a carousel on home page #3494

Closed suhail34 closed 1 year ago

suhail34 commented 1 year ago

Description

This PR fixes #3416

Notes for Reviewers Converted Stack of category filters to carousel using react-slick library

https://user-images.githubusercontent.com/77241166/205443864-148640a4-0329-4b9f-821f-306fa045d447.mp4

Signed commits

eeshaanSA commented 1 year ago

@Nikhil-Ladha can you enable the deployment-preview?

leecalcote commented 1 year ago

@franklinekoh, you might review this.

l5io commented 1 year ago

🚀 Preview for commit 5f9a3e9ec3cc72f2d9461d7266494ab9b1b8068c at: https://638d47b197124e15c66375cb--layer5.netlify.app

leecalcote commented 1 year ago

Some excellent feedback being given here, @kayceeDev and @franklinekoh.

suhail34 commented 1 year ago

Exams are on so was not able to see the review but made this changes today is this okay

https://user-images.githubusercontent.com/77241166/206634315-98fe295c-62a4-4173-9d16-dc0b783af06f.mp4

Also removed my styling from globalstyles and added it to component using styled-components

franklinekoh commented 1 year ago

@suhail34 good work. in addition to the above feedback, the carousel "next" and "previous" arrows should be horizontally aligned to match the filter category cards.

Screenshot 2022-12-10 at 08 13 35

I would suggest taking out the green background used on the carousel buttons and increase the size of the icons (< >). Then change Icon colours to green instead with no background.

Good luck with your exams man 👍

suhail34 commented 1 year ago

Is this Okay

Then i would commit my changes

https://user-images.githubusercontent.com/77241166/209069135-0c8fe800-8a13-4352-8a59-63301d6959ca.mp4

leecalcote commented 1 year ago

Merge conflict....

eeshaanSA commented 1 year ago

Great work, @suhail34 🚀🔥 Just address the merge conflict.

eeshaanSA commented 1 year ago

@suhail34 are you still around? :eyes:

leecalcote commented 1 year ago

@suhail34 there are a few merge conflicts to address, but let's see if we can give this a final review on tomorrow's Websites call. // @Chadha93 @eeshaanSA

l5io commented 1 year ago

🚀 Preview for commit 83ba013c5dc62676f26f054277dc8c321a07a559 at: https://63c4a7635eceb84acff89c34--layer5.netlify.app

suhail34 commented 1 year ago

yeah sure solved the merge conflict issue

l5io commented 1 year ago

🚀 Preview for commit adb043ffb734f4b3a56ffb369d2ff9448e498320 at: https://63c5586be8d7f119333f111c--layer5.netlify.app

l5io commented 1 year ago

🚀 Preview for commit 835039e1fb995cabe688886bbae7da5b3192213a at: https://63e106f6d7378e038063dbd1--layer5.netlify.app

l5io commented 1 year ago

🚀 Preview for commit 2be1f9625b955634f8cc1398259e7f84ee13ca42 at: https://63ea3a4450555711ae05b671--layer5.netlify.app

l5io commented 1 year ago

🚀 Preview for commit 31e832d04a889fdee3af73e98ac056bcf5b1b28e at: https://63ea6f64a9556f038ec8be89--layer5.netlify.app

eeshaanSA commented 1 year ago

@leecalcote looks fine on linux(chrome). image

leecalcote commented 1 year ago

To your point, @eeshaanSA, I don't see that same behavior in Chrome on macOS either. @suhail34, unfortunately, Safari is the most second most popular web browser and we'll need to ensure that is has first-class support on this site. If you don't have access to Safari, mention this in Slack and we'll point out others that do.

suhail34 commented 1 year ago

Hey @leecalcote could you just see if this works now or no

l5io commented 1 year ago

🚀 Preview for commit 3d459bd1b94f56118b60d0215154251d09901d5f at: https://63efc1a145276b067e1a54c7--layer5.netlify.app

leecalcote commented 1 year ago

@suhail34 I tried with the latest deploy preview. The same behavior persists in Safari.

suhail34 commented 1 year ago

then i would need a help with someone having macos

l5io commented 1 year ago

🚀 Preview for commit ff9a1ba382f529cf6d0c360ec211dee5417361c5 at: https://63f21f295175e46b16239c0b--layer5.netlify.app

l5io commented 1 year ago

🚀 Preview for commit 23b3453792d54558336e29a1a8bc5c1277dca54b at: https://63f7326c8465d7589cf6a7af--layer5.netlify.app

l5io commented 1 year ago

🚀 Preview for commit 34dfccf646c1a02f16d8e25a9c2b01afef58728d at: https://63f77b8fc3a7630d7aabdef9--layer5.netlify.app

suhail34 commented 1 year ago

does this works?

asubedy commented 1 year ago
image

In safari it renders like this. It's really hard to find a sweet spot so that both chrome and safari renders the arrow vertically centered aligned. I tried with different values but if one is aligned the other becomes not aligned.

suhail34 commented 1 year ago

can you try by adding 1rem to top in .slick-prev, .slick-next and remove height from .slick-arrow classes by inspecting

l5io commented 1 year ago

🚀 Preview for commit 87455f0c23eaa9ea7e9e7868445e77ed419ed948 at: https://63f9f6d5fb16ae0c8636e0a2--layer5.netlify.app

leecalcote commented 1 year ago

Screenshot from an iPhone. Both Chrome and Safari look the same way

283C01EB-A9C4-46B8-A3E4-11809DBFFE58

l5io commented 1 year ago

🚀 Preview for commit 2242b87c75816c1984f78e84ac65bc868eaf1ba5 at: https://63fa3a0035086a3c8c164385--layer5.netlify.app

Chadha93 commented 1 year ago

@suhail34 Let's discuss this on the Websites call. Please add this as an agenda item in the meeting minutes if you would. :)

l5io commented 1 year ago

🚀 Preview for commit f0117043c18195428771b622f742742854388d0a at: https://63fdd04852cf320098da1bc1--layer5.netlify.app

Nikhil-Ladha commented 1 year ago

Seems like it is compatible on every browser/device now. @leecalcote could you please confirm?

l5io commented 1 year ago

🚀 Preview for commit d14bab8e381e8d9eed45612b3c97303ddbede322 at: https://64034680f086d439116af928--layer5.netlify.app