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

Make showcase expandable (merged prematurely, see #889) #878

Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 24, 2024

Warning

All the discussion on changes happened here, but these aren't the exact commits that made it into the default branch. See #889 for those.

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 by making the showcase expandable.

Related issue(s)

Checklist

@victorlin victorlin self-assigned this May 24, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--ptoszr May 24, 2024 21:40 Inactive
@victorlin victorlin force-pushed the victorlin/make-showcase-expandable branch from 2e9bf43 to bcc4ac9 Compare May 24, 2024 21:41
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ptoszr May 24, 2024 21:41 Inactive
@victorlin victorlin marked this pull request as ready for review May 24, 2024 21:48
@victorlin victorlin requested review from a team and jameshadfield May 24, 2024 21:48
victorlin added 2 commits May 24, 2024 14:58
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 by making
the showcase expandable.
@victorlin victorlin force-pushed the victorlin/make-showcase-expandable branch from bcc4ac9 to a2e7b2a Compare May 24, 2024 22:26
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ptoszr May 24, 2024 22:26 Inactive
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

The spacing of the last row looks too much to me, aesthetically. What about centering the tiles so the spacing between them is always the same in every row?

image

static-site/src/components/ListResources/Showcase.tsx Outdated Show resolved Hide resolved
@genehack
Copy link
Contributor

+1 for approaching this differently; right now the grid can get real funky depending on viewport sizing and zoom level:

Screenshot 2024-05-24 at 15 55 29

This does feel like something CSS Grid could help with? (handy cheatsheet if you're not up-to-date on it)

@tsibley
Copy link
Member

tsibley commented May 24, 2024

This does feel like something CSS Grid could help with

I think flexbox is a more natural fit for what we want (which is not a fixed grid, unless I'm misunderstanding intent).

@genehack
Copy link
Contributor

This does feel like something CSS Grid could help with

I think flexbox is a more natural fit for what we want (which is not a fixed grid, unless I'm misunderstanding intent).

Yeah, either way I think; the big point is not re-inventing a wheel the browser should hand us. 😁

victorlin added 3 commits May 28, 2024 11:04
This is the default behavior of justify-content so an alternative is to
simply remove the line, but better to be explicit.

Note: there is a gap on the right side of the cards because the
collection of cards can't be centered with this approach. There are
workarounds¹ that don't use flexbox, but keeping it like this for now.

¹ <https://stackoverflow.com/q/32802202>
This was only necessary to determine if the container is expandable,
which can be determined from the height alone (already used elsewhere).
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ptoszr May 28, 2024 18:43 Inactive
@victorlin victorlin requested review from tsibley and genehack May 28, 2024 18:49
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ptoszr May 28, 2024 21:57 Inactive
@victorlin victorlin mentioned this pull request May 29, 2024
4 tasks
@trvrb trvrb self-requested a review May 29, 2024 21:35
@trvrb
Copy link
Member

trvrb commented May 29, 2024

Nice work @victorlin. I really like how this turned out. Appropriately snappy and the tile fading is good UI hint that there's content below the fold that can be unfurled.

Perhaps a separate issue from this PR. In the /pathogens page, at full width we have 9 total tiles and space for 8. This causes overflow for just Zika. The single tile overhang feels off. I'd either add a couple additional tiles or just drop one. If adding a couple, I'd add mumps and EV-D68.

@victorlin victorlin merged commit f7b770f into victorlin/list-resources-typing May 29, 2024
7 checks passed
@victorlin victorlin deleted the victorlin/make-showcase-expandable branch May 29, 2024 22:44
@victorlin
Copy link
Member Author

victorlin commented May 30, 2024

Whoops, I forgot this was based on #874, so these changes won't make it into master until that PR is merged.

EDIT: #874 is still in progress, so I'm re-opening this as #889 for a cleaner commit history.

@victorlin victorlin restored the victorlin/make-showcase-expandable branch May 31, 2024 18:05
@victorlin victorlin mentioned this pull request May 31, 2024
2 tasks
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--pfwtce May 31, 2024 18:09 Inactive
@victorlin victorlin mentioned this pull request Jun 18, 2024
15 tasks
victorlin added a commit that referenced this pull request Jun 21, 2024
This makes it easier to customize card components for future work. The
side effect is that the last row is centered, which was suggested
elsewhere¹.

¹ #878 (comment)
@victorlin victorlin mentioned this pull request Jun 21, 2024
2 tasks
@victorlin victorlin changed the title Make showcase expandable Make showcase expandable (merged prematurely, see #889) Jul 2, 2024
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.

6 participants