-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add marquee to website carousel component #322
base: temp
Are you sure you want to change the base?
Conversation
Signed-off-by: Vipul Gupta (@vipulgupta2048) <[email protected]>
Signed-off-by: Vipul Gupta (@vipulgupta2048) <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: Vipul Gupta (@vipulgupta2048) <[email protected]>
6f12849
to
055bc55
Compare
@rohan09-raj can you please review these changes? |
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.
Looks good to me, one question that does it handle scroll on user interaction to carousel ??
i dont think marquees let you scroll |
mb i fat fingered the close button |
I think when I tried it - it was scrolling? |
I mean you don't manually have to scroll it |
Shouldn't we allow the manual scrolling as well ? |
The React component has multiple issues open on users requesting draggable marquee or scrolling horizontal support: https://github.com/justin-chu/react-fast-marquee/issues?q=is%3Aissue+is%3Aopen+scroll |
It would be better to have carousels that autoscroll |
IMO, the improvement was to add an auto-scroll marquee component, which this PR added successfully. |
suggestions:
-+props={data.map((item) => {
+props={data.map((item, idx) => {
return (
<VolunteerItem
- key={item.id}
+ key={idx} there's a key error in one of them (maybe repeated id field in data sources) |
oh wait now that I'm seeing the original issue:
@Vyogami should the events section be a marquee or should it just remain a carousel? |
I suppose a carousel would make more sense! |
In addition to #323, the speed of marquee can be adjusted as atm the animation looks jittery it can be ease out to look more natural. |
Feel free to modify or tweak the changes if you want to, I might have some time next week to resolve these things. Changing the speed and removing events carasoul would be straightforward changes. |
Added react-marquee component to the Carousel
What has been done:
How it looks:
What's fixed: #319