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

Update people component #862

Merged
merged 2 commits into from
May 15, 2024
Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 15, 2024

Description of proposed changes

No functional changes – prepping for #860

Checklist

@victorlin victorlin self-assigned this May 15, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--ex1fdc May 15, 2024 22:53 Inactive
@victorlin victorlin mentioned this pull request May 15, 2024
9 tasks
@victorlin victorlin marked this pull request as ready for review May 15, 2024 23:01
@victorlin victorlin requested a review from a team May 15, 2024 23:01
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.

LGTM.

One minor thing I noted is that people without links are shown in a slightly different shade of black. Compare Pavel to Moira:

Screenshot from 2024-05-15 16-10-34

@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ex1fdc May 15, 2024 23:35 Inactive
The difference in usage required a lot of intermediate variables and
most of the code was unshared. Splitting gets rid of the intermediates
and unnecessary props (e.g. teamPage, comma).
@victorlin victorlin force-pushed the victorlin/update-people-component branch from 13d44a9 to 4047edc Compare May 15, 2024 23:39
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--ex1fdc May 15, 2024 23:39 Inactive
@victorlin
Copy link
Member Author

victorlin commented May 15, 2024

@tsibley thanks for noting that! I didn't realize there was already a member without link. I've updated the commit message from a8e591e to 4047edc.

The color difference is because (1) links here are set to a specific color and (2) the non-links don't have any color set via CSS. It seems like both our browsers default to pure black #000 in that case, so the link color #333 is slightly lighter. There's probably a few options to address this - I'll open a separate issue: #863

@victorlin victorlin merged commit d532048 into master May 15, 2024
6 checks passed
@victorlin victorlin deleted the victorlin/update-people-component branch May 15, 2024 23: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.

3 participants