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

Redesign showcase on splash #922

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jun 14, 2024

preview

image

Description of proposed changes

Adding more information to the showcase ("featured analyses") section.

Links

Checklist

  • Check if changes affect the resource index JSON revision
  • Settle on design
  • Settle on descriptions
  • Clean up code
  • Consider adding border color change on hover to mimic behavior of buttons 01bcb3c
    • The link underline on hover is a sufficient focus indicator for these cards.
  • Local: swapping in blab for a group URL shows the blab group logo
  • Checks pass
  • Merge base PR Swap splash tiles to use viz screenshots #914
  • Post-merge: check that the group logos are properly fetched (it's expected to not work on review app)

@victorlin victorlin self-assigned this Jun 14, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 14, 2024 22:02 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 14, 2024 22:47 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 15, 2024 00:23 Inactive
@victorlin victorlin mentioned this pull request Jun 18, 2024
15 tasks
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 18, 2024 23:21 Inactive
@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from 14b2ef4 to ec5da30 Compare June 18, 2024 23:26
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 18, 2024 23:26 Inactive
@trvrb trvrb temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 19, 2024 23:10 Inactive
@trvrb trvrb temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 19, 2024 23:40 Inactive
@trvrb
Copy link
Member

trvrb commented Jun 19, 2024

I really like where this is headed @victorlin (and it seems like most everyone agrees with this). I just made some minor tweaks. I'd be happy for this to go live except the issues with /pathogens, etc... and code clean up.

I think it's your call for whether to:

  1. Just merge this PR into PR Swap splash tiles to use viz screenshots #914 (branch trvrb/semantic-tiles) as this PR is set up in GitHub to do, and then clean up the PR there
  2. Close PR Swap splash tiles to use viz screenshots #914 and swap this PR to merge into master

@trvrb trvrb temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 20, 2024 00:01 Inactive
@victorlin
Copy link
Member Author

@trvrb thanks for those tweaks, visually it's looking very polished. One more thing I want to try is border change on hover to mimic behavior of buttons on the same page.

Code is quite messy so I'll clean it up with the aim to keep things visually identical.

Since there are additional screenshot updates in this PR, I'll close #914 and consolidate all the screenshot changes into this PR as a single commit.

@trvrb
Copy link
Member

trvrb commented Jun 20, 2024

Sounds good. Thanks @victorlin!

@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from fffe300 to 5e6764f Compare June 21, 2024 23:55
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 21, 2024 23:55 Inactive
@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from 5e6764f to afb144b Compare June 21, 2024 23:57
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 21, 2024 23:58 Inactive
@victorlin victorlin mentioned this pull request Jun 21, 2024
2 tasks
@victorlin victorlin changed the base branch from trvrb/semantic-tiles to victorlin/showcase-refactor June 21, 2024 23:59
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 22, 2024 00:13 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 24, 2024 20:56 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 24, 2024 21:26 Inactive
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 24, 2024 21:37 Inactive
@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from 32d2c7f to 3711b69 Compare June 24, 2024 22:46
@victorlin victorlin force-pushed the victorlin/showcase-refactor branch from cdce492 to e71d5e1 Compare June 24, 2024 22:46
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 24, 2024 22:46 Inactive
victorlin and others added 5 commits June 24, 2024 17:13
Co-authored-by: Trevor Bedford <trevor@bedford.io>
- Add description and type
- Ensure url is specific to match description
- Remove "(narrative)" since new design will show type information

Co-authored-by: Trevor Bedford <trevor@bedford.io>
Co-authored-by: Trevor Bedford <trevor@bedford.io>
Make each card bigger and display additional information.

Static preview: <https://github.com/nextstrain/nextstrain.org/assets/13424970/b04c0f6e-d35c-4502-bd6e-b893e90b66ba>

Co-authored-by: Trevor Bedford <trevor@bedford.io>
@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from 3711b69 to 08158d3 Compare June 25, 2024 00:13
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 25, 2024 00:14 Inactive
@victorlin victorlin changed the base branch from victorlin/showcase-refactor to trvrb/semantic-tiles June 25, 2024 00:17
@victorlin victorlin marked this pull request as ready for review June 25, 2024 00:23
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Thanks for all of the work you've put into the redesign @victorlin 🌈

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.

Really nice work @victorlin! Only one little bug which needs fixing (isNarrative prop), the rest of my comments can be split out to future work / discussion


useEffect(() => {
async function getGroupLogo(group) {
const response = await fetch(`/charon/getSourceInfo?prefix=/groups/${group}`);
Copy link
Member

Choose a reason for hiding this comment

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

Post-merge: check that the group logos are properly fetched (it's expected to not work on review app)

I've tested this locally with the appropriate IAM user + CONFIG_FILE="env/production/config.json" and it works.

[following is not a request for changes, just discussion]

These requests don't feel right to me. If we used SSR then they could be made (and cached) server-side, but fundamentally it feels like data that should be hardcoded similarly to how showcase.yaml is hardcoded. Maybe it's just fine, I don't know. Anyone else have thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hardcoding group logos in the YAML feels off to me. These aren't tracked in version control and can be changed by group owners at any time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoding group logos in the YAML feels off to me.

+1. I think the groups logo should be dynamic since we don't have control over them.

static-site/src/components/splash/index.jsx Show resolved Hide resolved
static-site/src/components/splash/index.jsx Outdated Show resolved Hide resolved
static-site/src/components/splash/index.jsx Show resolved Hide resolved
static-site/src/components/splash/index.jsx Show resolved Hide resolved
This removes the need to specify type in the YAML.
Copy link
Member

@trvrb trvrb left a comment

Choose a reason for hiding this comment

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

I really like where this ended up @victorlin. 😀

Ideally the code can be more flexible, but right now things are looking
good with the current content. Describe what sort of content is needed
to keep things looking good.
@victorlin victorlin force-pushed the victorlin/splash-showcase-redesign branch from bde50a3 to 13beec5 Compare June 27, 2024 18:07
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ubqpe8 June 27, 2024 18:08 Inactive
Base automatically changed from trvrb/semantic-tiles to master June 27, 2024 18:08
@victorlin victorlin merged commit 3f91b31 into master Jun 27, 2024
4 checks passed
@victorlin victorlin deleted the victorlin/splash-showcase-redesign branch June 27, 2024 18:09
@jameshadfield jameshadfield mentioned this pull request Aug 25, 2024
3 tasks
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