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.

Sign up for GitHub

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

Jump to bottom

Remove font-size scaling #1062

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove font-size scaling #1062

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions static-site/src/components/sourceInfoHeading.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ const OverviewContainer = styled.div`
width: 200px;
padding: 0;
}
a, p {
font-size: 94%;
}
Comment on lines -27 to -29
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why this was here in the first place?

(I mean, seems like we should remove it, but also, want to consider Chesterton's Fence and all.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't tell. It's from 347ee03 which was the commit that added the markdown rendering of group descriptions, so maybe it was a stylistic adjustment relative to whatever else was on the page at that time.

I can update the commit message to clarify this uncertainty (or certainty if someone else has a better explanation).

Copy link
Member

Choose a reason for hiding this comment

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

Nod, thanks for looking. I'd say it's worth adding that to the commit message, as well as a reason for the change in the commit message (i.e. font rendering issue reported by Emma, which you did link the PR). But not a blocker.

Copy link
Member

Choose a reason for hiding this comment

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

94% font-size is elsewhere in the code too (and was there in 347ee03 also):

/* lifted from auspice's default splash page */
function formatDataset(requestPath, dispatch, changePage) {
  return (
    <li key={requestPath}>
      <div
        style={{... fontSize: "94%"}}

Following the above comment back to Auspice, 94% font-size is used in 5 places. You could trace them back to see if there was a commit message explaining why, but I suspect there wasn't.

Given it's relatively widespread and long-term use, I don't understand why this could be responsible for any recent changes (unless we think it's been buggy for years but we only noticed recently?). Moreover, this comment in slack indicates the jagged-text went away when viewing next.nextstrain.org (with the app-router changes) and thus seems unrelated to the changes in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the PR description and commit message 943fa3e with a summary. The case for merging this is definitely weak and I'm fine with closing it out if it doesn't seem useful to others.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm +1 for merging it. As Victor notes in the commit message, it makes the overall text sizing more consistent, and the current scaling down doesn't appear to convey any semantic information. Additionally, I see a benefit (small, but potentially cumulative over time) from reducing the amount of CSS surface area in play — it's currently extremely hard to reason about CSS changes, and the only way I see to make it easier to try to reduce the amount that's in play.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm fine with this being merged. I'm extremely skeptical that this was the underlying reason for the original bug report on slack, as I explained above, but that doesn't prevent us from making this change.

`;

export const AvatarImg = styled.img`
Expand Down