Skip to content
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(Wallet): Vertical Slide #449

Merged
merged 20 commits into from
Oct 30, 2024
Merged

Conversation

DiogoSoaress
Copy link
Member

Copy link

github-actions bot commented Aug 28, 2024

Branch preview

✅ Deployed successfully in branch deployment:

https://wallet_vertical_slide--homepage.review.5afe.dev

@DiogoSoaress DiogoSoaress marked this pull request as ready for review August 29, 2024 13:41
Base automatically changed from wallet-big-cards to wallet-landing-page October 29, 2024 10:03
const [selectedIndex, setSelectedIndex] = useState(0)

const itemsImages = items.map((item) => item.image)
const icons = [<RecoveryIcon key="recovery" />, <ScanIcon key="scan" />, <MultipleKeysIcon key="multiple-keys" />]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we can move this outside of the component.

Comment on lines 49 to 50
{itemsImages[selectedIndex] ? (
<img src={itemsImages[selectedIndex].src} alt={itemsImages[selectedIndex].alt} className={css.image} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can extract itemsImages[selectedIndex] to a variable to simplify this.

// Change index every 5 seconds
useEffect(() => {
const interval = setInterval(() => {
setSelectedIndex((prevIndex) => (prevIndex + 1) % items?.length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return NaN if items is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initiate items to 0 in the received props

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, we don't need the optional chaining.

Comment on lines +18 to +20
const handleCardClick = (index: number) => {
setSelectedIndex(index)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is somewhat neglible. You can just call setSelectedIndex directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to leave the click handler

@DiogoSoaress DiogoSoaress merged commit 84abe05 into wallet-landing-page Oct 30, 2024
4 checks passed
@DiogoSoaress DiogoSoaress deleted the wallet-vertical-slide branch October 30, 2024 14:18
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants