-
Notifications
You must be signed in to change notification settings - Fork 48
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
Remove font-size scaling #1062
base: master
Are you sure you want to change the base?
Remove font-size scaling #1062
Conversation
8f94f86
to
522d69d
Compare
a, p { | ||
font-size: 94%; | ||
} |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
522d69d
to
d25e654
Compare
The purpose of the scaling is unclear. It was added in "parse /groups/:groupName markdown;" (347ee03) which is 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. A similar 94% font-size scaling is used in the custom Auspice splash page (auspice-client/customisations/splash.js) as well as other parts of the Auspice code base. The purposes of those are also unclear. The main motivation for removing it here is that there was an internal bug report¹ of a group's description rendering with what looks like partially scaled fonts. The issue self-resolved without any code changes and we were unable to understand what caused it. My suspicion is that this font-size scaling is a contributing factor. It can't be the only factor because it has been around for many years now and this is the first report of such an issue. Maybe a recent browser update handles scaled fonts poorly. Another motivation is that, after seeing the unscaled version, the scaled version has noticeably smaller text for what seems to be no apparent reason. I think removing the scaling improves readability on this page, but of course that is subjective. ¹ <https://bedfordlab.slack.com/archives/C0K3GS3J8/p1731343211138099>
d25e654
to
943fa3e
Compare
Description of proposed changes
The purpose of the scaling is unclear. It was added in "parse /groups/:groupName markdown;" (347ee03) which is 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.
A similar 94% font-size scaling is used in the custom Auspice splash page (auspice-client/customisations/splash.js) as well as other parts of the Auspice code base. The purposes of those are also unclear.
The main motivation for removing it here is that there was an internal bug report of a group's description rendering with what looks like partially scaled fonts. The issue self-resolved without any code changes and we were unable to understand what caused it. My suspicion is that this font-size scaling is a contributing factor. It can't be the only factor because it has been around for many years now and this is the first report of such an issue. Maybe a recent browser update handles scaled fonts poorly.
Another motivation is that, after seeing the unscaled version, the scaled version has noticeably smaller text for what seems to be no apparent reason. I think removing the scaling improves readability on this page, but of course that is subjective.
Related issues
Checklist