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

Basic carousel for new site #42

Merged
merged 17 commits into from
Nov 6, 2024
Merged

Basic carousel for new site #42

merged 17 commits into from
Nov 6, 2024

Conversation

nickbristow
Copy link
Collaborator

@nickbristow nickbristow commented Oct 25, 2024

Adds a basic carousel for the bottom of the homepage.

Also adds buttons for tabbing and keyboard control.
image

@nickbristow nickbristow marked this pull request as ready for review October 28, 2024 21:00
Copy link
Collaborator

@jakewheeler jakewheeler left a comment

Choose a reason for hiding this comment

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

Good work! Dropped a few initial thoughts here for you.

src/app/globals.css Outdated Show resolved Hide resolved
src/app/components/Carousel.tsx Outdated Show resolved Hide resolved
src/app/components/Carousel.tsx Outdated Show resolved Hide resolved
src/app/components/Carousel.tsx Outdated Show resolved Hide resolved
setFinalOffset(finalOffset + dragOffset); // Keep track of cumulative offset
setIsDragging(false);
setDragOffset(0); // Reset drag offset after drag ends
// setFinalOffset(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we delete this line? Was this just for testing?

src/app/globals.css Outdated Show resolved Hide resolved
src/app/components/Carousel.tsx Outdated Show resolved Hide resolved
}

.carousel-item {
min-width: calc(100% / 7); /* Adjust based on number of visible items */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this number intended to match itemsPerView?

Comment on lines 1 to 11
.sliderContainer {
h3 {
font-size: 36px;
margin: 10px;
padding: 2% 0;
background: var(--ifm-color-primary);
color: #fff;
line-height: 100px;
text-align: center;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need these styles? Looks like maybe not.

src/app/page.tsx Outdated
used DIBBs to solve their toughest data challenges
</p>
<>
<ContentContainer align classes="px-14 pt-20 pb-10">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please double check that your styling still works here once you merge #41 in 😃

Comment on lines 6 to 7
import 'slick-carousel/slick/slick.css';
import 'slick-carousel/slick/slick-theme.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could move these two imports into Carousel.tsx?

Copy link
Collaborator

@jakewheeler jakewheeler left a comment

Choose a reason for hiding this comment

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

Good work & thanks for the changes!

@nickbristow nickbristow merged commit 1df4f9f into next Nov 6, 2024
1 check passed
@nickbristow nickbristow deleted the carousel branch November 6, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants