Fix title inconsistencies #229

Merged

j285he merged 5 commits from jared-fix-headers into main 2 years ago

j285he commented 2 years ago

Collaborator

Closes #212.

j285he added 1 commit 2 years ago

b9f0d81c48 Fix inconsistencies

j285he added 1 commit 2 years ago

aba9a1bb4c Separate the issues

j285he changed title from ~~Fix header inconsistencies~~ to Fix title inconsistencies 2 years ago

a3thakra reviewed 2 years ago

pages/news/[year]/[term].module.css Outdated

a3thakra approved these changes 2 years ago

Dismissed

a3thakra left a comment

Collaborator

just 1 nit, lgtm otherwise

j285he added 1 commit 2 years ago

e491a4076d Pesky pixels

a3thakra reviewed 2 years ago

pages/events/[year]/[term]/index.tsx Outdated

													
					      {hasFutureEvents && (
				
					        <>
				
					          <h2>Upcoming Events</h2>
				
					          <h1>Upcoming Events</h1>

a3thakra commented 2 years ago

Collaborator

actually can you put this in a section tag? technically, there are two h1's on the page. But a page cannot have 2 h1s. If we look closely, we have two sections on this page, each with it's own h1. I think that would make this nicer. (in terms of semantics)

j285he commented 2 years ago

Poster

Collaborator

I currently have Past Events as an h2. Should I make it an h1?

a3thakra commented 2 years ago

Collaborator

Yes, semantically speaking, each page should have exactly one h1, and then 0 or more h2's

a3thakra commented 2 years ago

Collaborator

when i say h1, i mean the h1 tag. you can style it as you wish.

j285he commented 2 years ago

Poster

Collaborator

Ah okay. Also for the original comment, I don't see two h1's on the same page. It's just either Upcoming Events or Events Archive:. Am I going crazy?

a3thakra commented 2 years ago

Collaborator

"Upcoming events" and "Past events" should both be h1s enclosed within a section. So the final markup would look something like this:

<section>
    <h1>Upcoming events</h1>
    {events.map(...)}
</section>
<section>
    <h1>Past events</h1>
    {events.map(...)}
</section>

j285he commented 2 years ago

Poster

Collaborator

Ahhhhhh sorry I see what you mean, thanks!

Although what are your thoughts on the styling? I think having h2 on Past Events makes the user more drawn towards current upcoming events, instead of having two blocky h1's competing for attention, but idk

a3thakra commented 2 years ago

Collaborator

As I said earlier, they should be h1 tags, you can style them like h2s to match figma 😉

j285he commented 2 years ago

Poster

Collaborator

OK, thank you! Since Figma's title sizes differ a bit from ours, let me know if I should change the size of Past Events or not

a3thakra dismissed a3thakra’s review 2 years ago

create two sections on the events/term/index page.

j285he added 1 commit 2 years ago

87b5390a50 Add sections

a3thakra approved these changes 2 years ago

a3thakra left a comment

Collaborator

LGTM

j285he added 1 commit 2 years ago

30a0423adc Merge main

j285he merged commit 485230c95a into main 2 years ago

j285he referenced this issue from a commit 2 years ago

Fix title inconsistencies (#229)

j285he deleted branch jared-fix-headers 2 years ago

Reviewers

a3thakra approved these changes 2 years ago

continuous-integration/drone/push Build is passing

Details

The pull request has been merged as 485230c95a.

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#229

 .page > h1 {
   padding-bottom: calc(16rem / 16);
   border-bottom: 1px solid var(--primary-heading);