-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add landing page carousel #3407
base: feat/landing-page
Are you sure you want to change the base?
feat: add landing page carousel #3407
Conversation
strzelec seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start on this issue @adam-strzelec 👏
The landing page carousel adjustments look really nice 🤩
Screen.Recording.2024-10-22.at.10.29.15.mov
I have left a few refactoring comments and would highly appreciate if you can tackle them 💯
|
||
const descriptionMSG = defineMessages({ | ||
descriptionSlide0: { | ||
id: `${displayName}.dascriptionSlide0`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update id
to ${displayName}.descriptionSlide0
'From simple transactions to complex financial operations like Streaming, Milestone based, and Split payments, you can do it with Colony.', | ||
}, | ||
descriptionSlide1: { | ||
id: `${displayName}.dascriptionSlide0`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update id
to ${displayName}.descriptionSlide1
'Make bulk payments to different recipients using various tokens, amounts and scheduling. Saving time and reducing potential errors.', | ||
}, | ||
descriptionSlide2: { | ||
id: `${displayName}.dascriptionSlide0`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update id
to ${displayName}.descriptionSlide2
'sm:mr-4': !isImageFullWidth, | ||
'sm:w-[31.875rem]': !isImageFullWidth, | ||
'sm:min-w-[31.875rem]': !isImageFullWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we group all these classes into a single line
'sm:mr-4 sm:w-[31.875rem] sm:min-w-[31.875rem]': !isImageFullWidth
?
'aspect-[380/248]': !isImageFullWidth, | ||
'sm:aspect-[510/248]': !isImageFullWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here 'aspect-[380/248] sm:aspect-[510/248]': !isImageFullWidth
'py-[10px]': !isChangeSlideDotButton, | ||
'my-[-10px]': !isChangeSlideDotButton, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here 'py-[10px] my-[-10px]': !isChangeSlideDotButton
'w-[5.875rem]': !isChangeSlideDotButton, | ||
'h-[.1875rem]': !isChangeSlideDotButton, | ||
'mx-[.3125rem]': !isChangeSlideDotButton, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here 'w-[5.875rem] h-[.1875rem] mx-[.3125rem]': !isChangeSlideDotButton
const [emblaRef, emblaApi] = useEmblaCarousel( | ||
options, | ||
autoplay | ||
? [ | ||
Autoplay({ | ||
playOnInit: true, | ||
delay: 3000, | ||
}), | ||
] | ||
: [], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe refactor the plugins part as
const plugins: EmblaPluginType[] = [];
if (autoplay) {
plugins.push(
Autoplay({
playOnInit: true,
delay: 3000,
}),
)
}
const [emblaRef, emblaApi] = useEmblaCarousel(
options,
plugins,
);
thus making it clearer in the future if we plan on adding more plugins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job with this component @adam-strzelec 👍 however I would propose refactoring it as following, in order to store the messages in a single variable, then easily adjust the number of slides and their parsing.
Also it's always better to try to define a variable for storing a check rather than repeating it (currentSlide === index
) as if in the future there is a need to change the condition you might end up loosing track of the places this needs to be changed
import clsx from 'clsx';
import React, { useState } from 'react';
import { defineMessages, FormattedMessage } from 'react-intl';
import ImageCarousel from '~common/Extensions/ImageCarousel/ImageCarousel.tsx';
import Slide1 from '~images/assets/landing/slider1.png';
import Slide2 from '~images/assets/landing/slider2.png';
import Slide3 from '~images/assets/landing/slider3.png';
import SlideMobile from '~images/assets/landing/sliderMobile.png';
const displayName = 'frame.LandingPageCarousel';
const MSG = defineMessages({
titleSlide0: {
id: `${displayName}.titleSlide0`,
defaultMessage: 'A powerful, all-in-one payments suite',
},
titleSlide1: {
id: `${displayName}.titleSlide1`,
defaultMessage: 'Make bulk payments your way with ease',
},
titleSlide2: {
id: `${displayName}.titleSlide2`,
defaultMessage: 'Easily track & manage shared finances',
},
descriptionSlide0: {
id: `${displayName}.descriptionSlide0`,
defaultMessage:
'From simple transactions to complex financial operations like Streaming, Milestone based, and Split payments, you can do it with Colony.',
},
descriptionSlide1: {
id: `${displayName}.descriptionSlide1`,
defaultMessage:
'Make bulk payments to different recipients using various tokens, amounts and scheduling. Saving time and reducing potential errors.',
},
descriptionSlide2: {
id: `${displayName}.descriptionSlide2`,
defaultMessage:
'Transparency and clarity around shared finances is made simply with dashboard highlights, transaction history, and shared decision making. ',
},
});
const slides = [
{
title: MSG.titleSlide0,
description: MSG.descriptionSlide0,
},
{
title: MSG.titleSlide1,
description: MSG.descriptionSlide1,
},
{
title: MSG.titleSlide2,
description: MSG.descriptionSlide2,
},
];
const LandingPageCarousel = () => {
const [currentSlide, setCurrentSlide] = useState(0);
return (
<div>
<div className="w-full max-w-[31.25rem] md:hidden">
<img className="h-auto w-full" src={SlideMobile} alt="slider mobile" />
</div>
<div className="hidden w-full max-w-[31.25rem] overflow-hidden md:block">
<div className="relative mb-9 h-[7.25rem]">
{slides.map((slide, index) => {
const isCurrentSlide = currentSlide === index;
return (
<div className="absolute left-0 top-0">
<h1
className={clsx(
'transition-opacity duration-normal heading-2',
{
'opacity-100 delay-150': isCurrentSlide,
'opacity-0': !isCurrentSlide,
},
)}
>
<FormattedMessage {...slide.title} />
</h1>
<p
className={clsx(
'mt-[.875rem] text-md font-normal transition-opacity duration-normal',
{
'opacity-100 delay-150': isCurrentSlide,
'opacity-0': !isCurrentSlide,
},
)}
>
<FormattedMessage {...slide.description} />
</p>
</div>
);
})}
</div>
<ImageCarousel
slideUrls={[Slide1, Slide2, Slide3]}
options={{ align: 'center', loop: true }}
isImageFullWidth
isAutoplay
isChangeSlideDotButton={false}
setSelectedIndex={(currentSlideIndex) =>
setCurrentSlide(currentSlideIndex)
}
className="mx-[-30px] pb-[115px]"
/>
</div>
</div>
);
};
LandingPageCarousel.displayName = displayName;
export default LandingPageCarousel;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking quite good so far! Left a few improvement suggestions, but I see @mmioana already covered most of what I intended to drop comments on.
But overall, a nice implementation so far, seems quite extensible and I love that you've created a reusable component that doesn't do too much - ImageCarousel
and that you've decoupled the landing page on into a new one instead of just smashing everything in the LandingPage
component 👍
@@ -11,10 +11,18 @@ const displayName = 'common.Extensions.ImageCarousel'; | |||
const ImageCarousel: FC<ImageCarouselProps> = ({ | |||
slideUrls = images, | |||
options = { loop: true, align: 'start' }, | |||
isAutoplay = false, | |||
isImageFullWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we rework slideUrls
to be an array of image urls and classNames? would be a bit cleaner, since now we control all images from one variable
)} | ||
/> | ||
className={clsx('group', { | ||
'py-[10px]': !isChangeSlideDotButton, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just render a whole different thing if isChangeSlideDotButton
is false?
{scrollSnaps.map((_, index) => | ||
isChangeSlideDotButton ? ( | ||
<DotButton | ||
// eslint-disable-next-line react/no-array-index-key | ||
key={index} | ||
onClick={() => scrollTo(index)} | ||
className={clsx( | ||
'mx-1 h-2 w-2 cursor-pointer rounded-full bg-gray-200 transition-all duration-normal hover:bg-blue-400', | ||
{ | ||
'bg-gray-500': index === selectedIndex, | ||
}, | ||
)} | ||
/> | ||
) : ( | ||
<button | ||
type="button" | ||
aria-label={formatMessage({ id: 'ariaLabel.changeSlide' })} | ||
// eslint-disable-next-line react/no-array-index-key | ||
key={index} | ||
onClick={() => scrollTo(index)} | ||
className="group my-[-10px] py-[10px]" | ||
> | ||
<div | ||
className={clsx( | ||
'mx-1 mx-[.3125rem] h-2 h-[.1875rem] w-2 w-[5.875rem] cursor-pointer rounded-full bg-gray-200 transition-all duration-normal group-hover:bg-blue-400', | ||
{ | ||
'bg-gray-500': index === selectedIndex, | ||
}, | ||
)} | ||
/> | ||
</button> | ||
), | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a very small change here @adam-strzelec if you could please add a constant
const isSelectedIndex = index === selectedIndex
and replace the usage of index === selectedIndex
with isSelectedIndex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @adam-strzelec and thanks so much for your fast changes 💯 🙏
Everything still looks good and works as expected 🎉
Screen.Recording.2024-10-23.at.09.41.33.mov
Left a very small refactoring suggestion, but definitely not a blocker for merging this PR, so feel free to decide whether to tackle it or not 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rdig , I understand that 3 approvals are required to merge the PR, but if we don’t receive 3 reviews (and approvals) by the end of tomorrow, is there any issues with going ahead and merging this PR? The deadline for submitting the Landing Page Updates is around November 20th, and I know your team has a conference planned, which will likely slow down the CR process. I would like this to apply to @adam-strzelec other PRs related to the Landing Page Updates as well, to ensure that we stay on track. |
Description
Create carousel for landing page
Testing
Landing Page Carousel
Diffs
New stuff ✨
Screen.Recording.2024-10-21.at.13.59.45.mov
Main issue - #3392