TechTalk Card + mini tech card #71
Merged
c29wan
merged 18 commits from feat/tech-talk-card into main 2 years ago
a3thakra
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'feat/tech-talk-card'
Deleting a branch is permanent. It CANNOT be undone. Continue?
fixes #35 and #34
TechTalk Cardto TechTalk Card + mini tech card 2 years agoLooks good to me overall, just a few small things.
I looked through the css, but I'll let @a3thakra review it as well.
<section className={styles.card}><aside>{poster && <Image alt={name} src={poster} />}{!poster && <div className={styles.spacer}></div>}Similar to the MiniTechTalkCard, I think we can remove the spacer for now, and think about what it should look like when we actually import the data and render the whole site.
import { TeamMemberCard } from "./TeamMemberCard";import { OrganizedContent, LinkProps } from "./OrganizedContent";import { Button } from "./Button";import { Footer } from "./Footer";This import is unused.
#1481e3 -17.95%,#4ed4b2 172.82%);--background-teal-2: rgb(78, 212, 178, 0.2);Nitpick: Can you move this to just below the
--teal-2-20line, and rename it to--teal-2-background, for consistency?.card {display: flex;flex-direction: row;max-width: calc(800rem / 16);You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component
.content h1 {margin-block-start: 4px;margin: 0;You can do:
instead of
margin-block-startandmargin.box-sizing: border-box;padding: calc(16rem / 16);color: var(--purple-2);font-size: calc(14rem / 16);Great attention to detail on the font-size here!! But I think that the design folks forgot to update the font-size to 16px (or 1rem) a while back. So you should change it to 1 rem :)
Added a comment on figma re the mini tech talk card.
export function TechTalkCard({ name, poster, children }: TechTalkProps) {return (<article><section className={styles.card}>You dont need to add a section tag inside an article. You can just do
<article className={styles.card}>and not have the section tag on line 15.{poster && <Image alt={name} src={poster} />}{!poster && <div className={styles.spacer}></div>}</aside><section className={styles.content}>We should use sections inside an article to separate content. But this is the main content of the article! So you should just wrap it up with a div instead of using a section tag here. :)
flex-direction: row;max-width: calc(800rem / 16);box-sizing: border-box;padding: calc(24rem / 16);I dont think you need this padding.
.card {display: flex;flex-direction: row;max-width: calc(800rem / 16);You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component
flex-direction: column;max-width: calc(1200rem / 16);box-sizing: border-box;padding: calc(24rem / 16);you dont need to reapply all the styles again. In CSS (Cascading Style Sheets) rules matching the same element "cascade" on top of each other. Think of it like a layered system.
So all you would need to do in here would be
flex-direction: column. Everything else can be removed :)return (<article className={styles.card}><aside>{poster && <Image alt={name} src={poster} />}</aside><section className={styles.content}>a div over here, instead of a section would be preferred.
You should use sections inside an article only if articles are big enough.
interface TechTalkProps {name: string;short: string;We don't need short inside a tech talk card.
Only two nit-picks, good to go after that!!
1fb308327finto main 2 years agoReviewers
1fb308327f.