www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

Standardize font sizes #46

Merged
j285he merged 12 commits from j285he-standardize-font-sizes into main 2022-09-12 10:08:02 -04:00
Conversation 3 Commits 12 Files Changed 2 +45 -9
j285he commented 2022-08-31 23:22:31 -04:00
Member
Copy Link

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 2022-08-31 23:22:32 -04:00
continuous-integration/drone/push Build is passing Details
4e79ac5de2 Standardize font sizes
j285he requested review from a258wang 2022-08-31 23:24:14 -04:00
j285he requested review from snedadah 2022-08-31 23:24:16 -04:00
j285he requested review from b72zhou 2022-08-31 23:24:25 -04:00
j285he requested review from e26chiu 2022-08-31 23:24:28 -04:00
j285he added 1 commit 2022-09-01 01:29:42 -04:00
continuous-integration/drone/push Build is passing Details
3760850040 Scale font sizes by 0.8 (Figma design was too large)
j285he added 1 commit 2022-09-01 01:41:27 -04:00
continuous-integration/drone/push Build is passing Details
bdfaa0372b Add font size for a
snedadah added 1 commit 2022-09-02 17:49:12 -04:00
snedadah reviewed 2022-09-02 17:58:23 -04:00
pages/_app.css Outdated
@ -72,1 +71,3 @@
h2 {
/* Major section headings (e.g. About the Programs) */
h1 {
snedadah commented 2022-09-02 17:57:52 -04:00
Owner
Copy Link

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.
a258wang commented 2022-09-03 00:31:00 -04:00
Owner
Copy Link

+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 2022-09-02 18:04:28 -04:00
continuous-integration/drone/push Build is passing Details
084b51ed7a
Fixed merge error and changed color
snedadah reviewed 2022-09-02 18:05:31 -04:00
pages/_app.css Outdated
@ -76,0 +78,4 @@
/* Page titles (e.g. Demographics) */
h2 {
font-size: calc(56rem / 16);
color: var(--primary-accent-light);
snedadah commented 2022-09-02 18:05:31 -04:00
Owner
Copy Link

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.
a258wang commented 2022-09-03 00:35:10 -04:00
Owner
Copy Link

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 2022-09-03 00:45:14 -04:00
a258wang left a comment
Owner
Copy Link

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
@ -85,3 +99,4 @@
}
a {
font-size: calc(24rem / 16);
a258wang commented 2022-09-03 00:38:18 -04:00
Owner
Copy Link

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 2022-09-07 20:43:45 -04:00
continuous-integration/drone/push Build is passing Details
9a22b7c56e Code review fixes
j285he commented 2022-09-07 20:44:35 -04:00
Author
Member
Copy Link

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.
👍 1
j285he added 1 commit 2022-09-07 23:04:36 -04:00
continuous-integration/drone/push Build is passing Details
827ae0dcf3 Merge branch 'main' into j285he-standardize-font-sizes
j285he added 1 commit 2022-09-07 23:05:38 -04:00
continuous-integration/drone/push Build is passing Details
f3ad60035f Merge main
a258wang reviewed 2022-09-09 12:25:58 -04:00
pages/_app.css Outdated
@ -71,1 +71,4 @@
margin: 0;
/* Font styling for body */
font-size: calc(22rem / 16);
a258wang commented 2022-09-09 12:24:17 -04:00
Owner
Copy Link

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. 🙂)
j285he commented 2022-09-09 17:17:18 -04:00
Author
Member
Copy Link

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
❤️ 1
pages/_app.css Outdated
@ -72,0 +77,4 @@
/* Page titles (e.g. Demographics) */
h1 {
font-size: calc(56rem / 16);
a258wang commented 2022-09-09 12:25:10 -04:00
Owner
Copy Link

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
pages/playground.tsx
@ -33,0 +38,4 @@
<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>
a258wang commented 2022-09-09 12:25:43 -04:00
Owner
Copy Link

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
a258wang commented 2022-09-09 12:36:44 -04:00
Owner
Copy Link

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 2022-09-09 17:01:36 -04:00
Add Textbox Component (#45)
j285he added 2 commits 2022-09-09 17:15:15 -04:00
continuous-integration/drone/push Build is passing Details
0b7b3881e7 Code review fixes
snedadah commented 2022-09-09 18:14:01 -04:00
Owner
Copy Link

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 2022-09-10 00:54:03 -04:00
a258wang left a comment
Owner
Copy Link

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
@ -71,1 +71,4 @@
margin: 0;
/* Font styling for body */
font-size: 1rem;
a258wang commented 2022-09-10 00:53:28 -04:00
Owner
Copy Link

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 2022-09-12 10:02:10 -04:00
continuous-integration/drone/push Build is passing Details
c4b9824a1e 16px -> 18px
j285he merged commit 3cb5780964 into main 2022-09-12 10:08:02 -04:00
j285he deleted branch j285he-standardize-font-sizes 2022-09-12 10:08:02 -04:00
j285he referenced this issue from a commit 2022-09-12 10:08:03 -04:00
Standardize font sizes (#46)
j285he referenced this issue from a commit 2022-09-12 10:20:11 -04:00
Add About Component (#48)
j285he referenced this issue from a commit 2022-09-12 20:07:05 -04:00
Add Sections Component (#49)
Sign in to join this conversation.
Reviewers
No reviewers
snedadah
b72zhou
e26chiu
a258wang
Labels
Clear labels   
Bug

   
Component

   
Config

   
Good First Issue

   
Low-Priority

   
Page
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#46
No description provided.
Delete Branch "j285he-standardize-font-sizes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?