Standardize font sizes #46

Merged
j285he merged 12 commits from j285he-standardize-font-sizes into main 5 months ago
Collaborator

Closes #33.

There were many more font sizes in the Figma, but as a best design practice, I chose the four most common.

Staging:

https://j285he-standardize-font-sizes-csc-class-prof-snedadah.k8s.csclub.cloud/

Closes #33. There were many more font sizes in the Figma, but as a best design practice, I chose the four most common. Staging: https://j285he-standardize-font-sizes-csc-class-prof-snedadah.k8s.csclub.cloud/
j285he added 1 commit 5 months ago
4e79ac5de2 Standardize font sizes
j285he requested review from a258wang 5 months ago
j285he requested review from snedadah 5 months ago
j285he requested review from b72zhou 5 months ago
j285he requested review from e26chiu 5 months ago
j285he added 1 commit 5 months ago
3760850040 Scale font sizes by 0.8 (Figma design was too large)
j285he added 1 commit 5 months ago
bdfaa0372b Add font size for a
snedadah added 1 commit 5 months ago
snedadah reviewed 5 months ago
pages/_app.css Outdated
h2 {
/* Major section headings (e.g. About the Programs) */
h1 {
Owner

I think it would make more sense to rename this h2, and make the next one h1 instead. Since it is a bit weird in my opinion how the h2 font size is bigger than this one. Furthermore, in terms of semantic sense, something like a page title makes more sense as an h1, while a section heading makes more sense as h2.

I think it would make more sense to rename this h2, and make the next one h1 instead. Since it is a bit weird in my opinion how the h2 font size is bigger than this one. Furthermore, in terms of semantic sense, something like a page title makes more sense as an h1, while a section heading makes more sense as h2.
Owner

+1, h1 should be larger than h2, in my opinion. I also agree that page titles should probably be h1, and section headings should probably be h2.

+1, h1 should be larger than h2, in my opinion. I also agree that page titles should probably be h1, and section headings should probably be h2.
j285he marked this conversation as resolved
snedadah added 1 commit 5 months ago
084b51ed7a
Fixed merge error and changed color
snedadah reviewed 5 months ago
pages/_app.css Outdated
/* Page titles (e.g. Demographics) */
h2 {
font-size: calc(56rem / 16);
color: var(--primary-accent-light);
Owner

I changed this to -primary-accent-light from -primary-accent-lighter since I think it better matches the figma design of "demographics". Please change it back if you think I'm mistaken.

I changed this to `-primary-accent-light` from `-primary-accent-lighter` since I think it better matches the figma design of "demographics". Please change it back if you think I'm mistaken.
Owner

Personally, I'm not quite as fussed about the shades of colour at this point, since
(a) it's not too hard to change which heading is which colour later, and
(b) I'm considering doing an audit of all the colours at the end anyways, so we might end up tweaking the hex strings slightly (eg. making sure text vs. background colour contrast is sufficient, and aso primary-accent-light and secondary-accent-light can be kinda hard to tell apart sometimes imo).

Personally, I'm not quite as fussed about the shades of colour at this point, since (a) it's not too hard to change which heading is which colour later, and (b) I'm considering doing an audit of all the colours at the end anyways, so we might end up tweaking the hex strings slightly (eg. making sure text vs. background colour contrast is sufficient, and aso primary-accent-light and secondary-accent-light can be kinda hard to tell apart sometimes imo).
j285he marked this conversation as resolved
a258wang requested changes 5 months ago
a258wang left a comment
Owner

Some requests:

  • Get rid of font-size for p, and add a sensible default to body instead. I'd suggest font-size: 1rem.
  • Specify a default font-weight under body. It seems like all the text in Figma is bold (700), but I'd suggest maybe 500 or 600 (or 700) by default under body, then 700 for all h1, h2, ..., h6.
  • Specify some sensible top/bottom margins for each of h1, h2, ..., h6, as well as p. See the website for examples.
  • It might be worth considering what the heading font-sizes/margins should be on mobile? We can also put that off until later though.
Some requests: - Get rid of `font-size` for `p`, and add a sensible default to `body` instead. I'd suggest `font-size: 1rem`. - Specify a default `font-weight` under `body`. It seems like all the text in Figma is bold (700), but I'd suggest maybe 500 or 600 (or 700) by default under `body`, then 700 for all h1, h2, ..., h6. - Specify some sensible top/bottom margins for each of h1, h2, ..., h6, as well as p. See [the website](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/pages/_app.css#L190) for examples. - It might be worth considering what the heading font-sizes/margins should be on mobile? We can also put that off until later though.
pages/_app.css Outdated
}
a {
font-size: calc(24rem / 16);
Owner

Let's get rid of this and set font-size: 1rem by default under body? It feels a little wack for anchors (a tags) to potentially have a different font size from the surrounding text, in my opinion. If specific anchors need to be larger, we can used CSS modules to style just those anchors.

Let's get rid of this and set `font-size: 1rem` by default under `body`? It feels a little wack for anchors (`a` tags) to potentially have a different font size from the surrounding text, in my opinion. If specific anchors need to be larger, we can used CSS modules to style just those anchors.
j285he marked this conversation as resolved
j285he added 2 commits 5 months ago
9a22b7c56e Code review fixes
Poster
Collaborator

I just copied the website margins over.

Also deleted h5, h6 since I think that those aren't being used and it's good to limit the amount of different font-sizes. Let me know if you want that reinstated.

I just copied the website margins over. Also deleted h5, h6 since I think that those aren't being used and it's good to limit the amount of different font-sizes. Let me know if you want that reinstated.
j285he added 1 commit 5 months ago
827ae0dcf3 Merge branch 'main' into j285he-standardize-font-sizes
j285he added 1 commit 5 months ago
f3ad60035f Merge main
a258wang reviewed 5 months ago
pages/_app.css Outdated
margin: 0;
/* Font styling for body */
font-size: calc(22rem / 16);
Owner

I personally think this body font size (and all the font sizes here in general) are quite large. For comparison, on the website, our page titles (eg. About Us) are 48px, and body text is 16px (1rem).

Is there a reason for these numbers? Otherwise I'd personally prefer bumping everything down a bit, eg.
p = 1rem
h1 = 48px
h2 = 36px
h3 = anything between 24px - 32px (inclusive), you can pick what looks best
h4 = 24px (same as what we currently have)
(ofc these should all be calc(...rem / 16) etc. 🙂)

I personally think this body font size (and all the font sizes here in general) are quite large. For comparison, on the website, our page titles (eg. About Us) are 48px, and body text is 16px (1rem). Is there a reason for these numbers? Otherwise I'd personally prefer bumping everything down a bit, eg. p = 1rem h1 = 48px h2 = 36px h3 = anything between 24px - 32px (inclusive), you can pick what looks best h4 = 24px (same as what we currently have) (ofc these should all be `calc(...rem / 16)` etc. 🙂)
Poster
Collaborator

I think the p is a touch small but perhaps that might just be my screen.

I've updated to these above specifications and we can always change back if needed. Thanks

I think the `p` is a touch small but perhaps that might just be my screen. I've updated to these above specifications and we can always change back if needed. Thanks
pages/_app.css Outdated
/* Page titles (e.g. Demographics) */
h1 {
font-size: calc(56rem / 16);
Owner

NIT: set font-weight for all headers too? could either do

h1, h2, h3, h4 {
  font-weight: 700;
}

or just add it to the styles for each header.

NIT: set font-weight for all headers too? could either do ```css h1, h2, h3, h4 { font-weight: 700; } ``` or just add it to the styles for each header.
j285he marked this conversation as resolved
<h3>h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3 h3</h3>
<h4>h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4 h4</h4>
<p>p p p p p p p p p p p p p p p p p p p p p p p p p p p p</p>
<a>a a a a a a a a a a a a a a a a a a a a a a a a a a a a</a>
Owner

NIT: add href="#" or something so we get the pointer cursor when hovering over the link :))

NIT: add `href="#"` or something so we get the pointer cursor when hovering over the link :))
j285he marked this conversation as resolved
Owner

Also not sure why, but the staging link doesn't seem to work for me

https://j285he-standardize-font-sizes-csc-class-profile-staging-snedadah.k8s.csclub.cloud

I was able to build and run the site locally though.

Also not sure why, but the staging link doesn't seem to work for me https://j285he-standardize-font-sizes-csc-class-profile-staging-snedadah.k8s.csclub.cloud I was able to build and run the site locally though.
j285he referenced this issue from a commit 5 months ago
j285he added 2 commits 5 months ago
Owner

Also not sure why, but the staging link doesn't seem to work for me

https://j285he-standardize-font-sizes-csc-class-profile-staging-snedadah.k8s.csclub.cloud

I was able to build and run the site locally though.

Sorry, staging was broken since the name was too long. I added a max length cutoff now:

Here is a working link:

https://j285he-standardize-font-sizes-csc-class-prof-snedadah.k8s.csclub.cloud/

> Also not sure why, but the staging link doesn't seem to work for me > > https://j285he-standardize-font-sizes-csc-class-profile-staging-snedadah.k8s.csclub.cloud > > I was able to build and run the site locally though. Sorry, staging was broken since the name was too long. I added a max length cutoff now: Here is a working link: https://j285he-standardize-font-sizes-csc-class-prof-snedadah.k8s.csclub.cloud/
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

Thanks for getting this done! One last comment but overall it looks good to me 🙂

Thanks for getting this done! One last comment but overall it looks good to me 🙂
pages/_app.css Outdated
margin: 0;
/* Font styling for body */
font-size: 1rem;
Owner

Okay yeah, idk why but 16px does feel a little small... I tried 18px and I think it looks good? Up to you, we can always change it later.

Okay yeah, idk why but 16px does feel a little small... I tried 18px and I think it looks good? Up to you, we can always change it later.
j285he added 1 commit 5 months ago
c4b9824a1e 16px -> 18px
j285he merged commit 3cb5780964 into main 5 months ago
j285he deleted branch j285he-standardize-font-sizes 5 months ago
j285he referenced this issue from a commit 5 months ago
j285he referenced this issue from a commit 5 months ago
j285he referenced this issue from a commit 5 months ago

Reviewers

snedadah was requested for review 5 months ago
b72zhou was requested for review 5 months ago
e26chiu was requested for review 5 months ago
a258wang approved these changes 5 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 3cb5780964.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#46
Loading…
There is no content yet.