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
2 changed files with 45 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
pages/_app.css
View File

@ -69,22 +69,52 @@ body {
color: var(--primary-text);
font-family: "Inconsolata", monospace;
margin: 0;
/* Font styling for body */
j285he marked this conversation as resolved Outdated
snedadah commented 2022-09-02 17:57:52 -04:00
Outdated
Review
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
Outdated
Review
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.
font-size: calc(18rem / 16);
a258wang commented 2022-09-09 12:24:17 -04:00
Outdated
Review
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
Outdated
Review
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
a258wang commented 2022-09-10 00:53:28 -04:00
Outdated
Review
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.
font-weight: 500;
}
h1,
/* Page titles (e.g. Demographics) */
h1 {
font-size: calc(48rem / 16);
j285he marked this conversation as resolved Outdated
a258wang commented 2022-09-09 12:25:10 -04:00
Outdated
Review
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.
font-weight: 700;
j285he marked this conversation as resolved Outdated
snedadah commented 2022-09-02 18:05:31 -04:00
Outdated
Review
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
Outdated
Review
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).
color: var(--primary-accent-light);
margin-top: calc(32rem / 16);
margin-bottom: calc(16rem / 16);
}
/* Major section headings (e.g. About the Programs) */
h2 {
font-size: calc(36rem / 16);
font-weight: 700;
color: var(--primary-heading);
margin-top: calc(32rem / 16);
margin-bottom: calc(16rem / 16);
}
/* Minor section headings & questions (e.g. What Program Are You In?) */
h3 {
font-size: calc(32rem / 16);
font-weight: 700;
color: var(--secondary-heading);
font-size: calc(45rem / 16);
margin-top: calc(24rem / 16);
margin-bottom: calc(16rem / 16);
j285he marked this conversation as resolved Outdated
a258wang commented 2022-09-03 00:38:18 -04:00
Outdated
Review
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.
}
h4,
h5,
h6 {
/* Titles within paragraphs (e.g. About the Programs -> Computer Science)*/
h4 {
font-size: calc(24rem / 16);
font-weight: 700;
color: var(--secondary-heading);
margin-top: calc(24rem / 16);
margin-bottom: calc(8rem / 16);
}
p {
color: var(--primary-text);
margin-top: 1rem;
margin-bottom: 1rem;
}
a {
@ -96,10 +126,6 @@ a:hover {
color: var(--link-hover);
}
p {
font-size: calc(28rem / 16);
}
@media only screen and (prefers-color-scheme: dark) {
body {
--primary-background: var(--dark--primary-background);

10
pages/playground.tsx
View File

@ -31,6 +31,16 @@ export default function Home() {
<PieChart data={mockPieData} width={800} labelWidth={215} />
</div>
<ColorPalette />
<h2>
<code>Text Styles</code>
</h2>
<h1>h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1 h1</h1>
<h2>h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2 h2</h2>
<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>
j285he marked this conversation as resolved
a258wang commented 2022-09-09 12:25:43 -04:00
Review
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 :))
<a href="#">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>
<h2>
<code>{"<BarGraphHorizontal />"}</code>
</h2>