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

Add carousel to /pathogens showcase #873

Closed
wants to merge 2 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 21, 2024

preview

Description of proposed changes

Previously, the showcase would drop cards as the width decreases. This is presumably to prevent the showcase from taking up too much height. Instead of hiding the unused cards, allow them to be accessed in a carousel via scroll/arrow clicking.

Related issue(s)

Checklist

victorlin added 2 commits May 21, 2024 15:01
Other carousel packages with reasons why I didn't choose them over
react-multi-carousel:

- react-slick: has 5 dependencies vs. 0 dependencies
- react-elastic-carousel: last published 3 years ago
- react-responsive-carousel: doesn't support displaying multiple items
  at a time, also not actively maintained
Previously, the showcase would drop cards as the width decreases. This
is presumably to prevent the showcase from taking up too much height.
Instead of hiding the unused cards, allow them to be accessed in a
carousel via scroll/arrow clicking.
@victorlin victorlin self-assigned this May 21, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--phhwcc May 21, 2024 22:04 Inactive
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Tested out briefly and looks great at a glance. The arrows currently obscure the titles at the moment (or vice versa?)
image

@tsibley
Copy link
Member

tsibley commented May 21, 2024

There's some non-ideal behaviour going on with the responsive widths of the tiles at some viewport widths:

image

If I make the window a little bit smaller or little bit wider it looks fine.

))}
</ShowcaseContainer>
</SingleRow>
<Carousel responsive={responsiveOptions} renderButtonGroupOutside={true}>
Copy link
Member Author

Choose a reason for hiding this comment

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

@jameshadfield re: #873 (review)

The arrows currently obscure the titles at the moment (or vice versa?)

There's props to customize the arrows which I'll look into. I tried briefly with renderButtonGroupOutside but looks like more options are necessary to get it working properly.

Comment on lines +16 to +32
// Generate options for carousel
let responsiveOptions = {};

for (let i = 0; i < cards.length; i++) {
const breakpointMin = i * cardWidthHeight;
let breakpointMax = (i + 1) * cardWidthHeight;

// Don't limit max otherwise carousel won't render at all on larger screens
if (i == cards.length - 1) {
breakpointMax = 999999
}

responsiveOptions[i] = {
breakpoint: { max: breakpointMax, min: breakpointMin },
items: i
};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@tsibley re: #873 (comment)

There's some non-ideal behaviour going on with the responsive widths of the tiles at some viewport widths

Unfortunately react-multi-carousel takes a fixed number of items opposed to a fixed width per item, which I would prefer avoids the behavior you'r seeing. With a fixed number of items, there's bound to be some stretching and contracting. I did a quick and dirty calculation here – will tweak it to contract a bit less.

Copy link
Member

Choose a reason for hiding this comment

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

Can you use a flexbox to evenly space them? That seems better than stretching / contracting cards

Copy link
Member

Choose a reason for hiding this comment

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

I presume the bigger issue behind this is that we're not observing / responding to viewport changes (this is from memory... we do observe this elsewhere in the resource listing UI).

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that, the viewport observation is flowing into the carousel. But if we've scrolled (?) to the end of the carousel and then expand the page it doesn't respond appropriately. I'd consider this minor / don't need to fix:

image

A separate issue is the number of cards displayed for a given width, which is causing them to become rectangles:

image

@trvrb
Copy link
Member

trvrb commented May 22, 2024

Thanks for diving in here @victorlin! This might be personal bias, but I'm not a big fan of carousel UI on almost any website. In this case especially, you have to do a lot of clicking to reveal the additional tiles one at a time.

My guess is that a nicer UI would be a dropdown carrot that expands (and collapses back) from a single row of tiles to a grid of tiles showing everything. This way initial view of tiles doesn't eclipse the cards, but you can expand to see everything if you'd like.

@victorlin
Copy link
Member Author

@trvrb thanks for the feedback, I'll explore that idea with a separate PR so we can compare the two. Note that we also plan to use this on the splash page at https://nextstrain.org.

@tsibley
Copy link
Member

tsibley commented May 22, 2024

My guess is that a nicer UI would be a dropdown carrot that expands (and collapses back) from a single row of tiles to a grid of tiles showing everything. This way initial view of tiles doesn't eclipse the cards, but you can expand to see everything if you'd like.

I like that idea. Something like the following (bad) mockup?

expand

@victorlin
Copy link
Member Author

Closing in favor of #878

@victorlin victorlin closed this May 30, 2024
@victorlin victorlin deleted the victorlin/showcase-carousel branch May 30, 2024 16:50
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.

5 participants