www
/
www-new
12
3
Fork 5
Code Issues 58 Pull Requests 42 Projects Releases Wiki Activity

TechTalk Card + mini tech card #71

Merged
c29wan merged 18 commits from feat/tech-talk-card into main 2021-07-29 16:56:48 -04:00
Conversation 2 Commits 18 Files Changed 10 +210 -2
c29wan commented 2021-06-21 18:50:17 -04:00 (Migrated from git.uwaterloo.ca)
Copy Link

fixes #35 and #34

  • extra padding below poster image
fixes #35 and #34 - extra padding below poster image
c29wan added 1 commit 2021-07-02 15:23:45 -04:00
c29wan added 1 commit 2021-07-02 16:02:59 -04:00
c29wan added 1 commit 2021-07-02 16:08:18 -04:00
continuous-integration/drone/push Build is passing Details
b5a6a50ddf Merge branch 'main' into feat/tech-talk-card
c29wan changed title from TechTalk Card to TechTalk Card + mini tech card 2021-07-02 16:17:41 -04:00
c29wan added 1 commit 2021-07-08 17:58:36 -04:00
continuous-integration/drone/push Build is passing Details
280adff67f Merge branch 'main' into feat/tech-talk-card
c29wan added 2 commits 2021-07-08 18:31:35 -04:00
n3parikh reviewed 2021-07-12 17:58:53 -04:00
n3parikh left a comment
Owner
Copy Link

Looks good to me overall, just a few small things.

I looked through the css, but I'll let @a3thakra review it as well.

Looks good to me overall, just a few small things. I looked through the css, but I'll let @a3thakra review it as well.
components/TechTalkCard.tsx Outdated
@ -0,0 +15,4 @@
<section className={styles.card}>
<aside>
{poster && <Image alt={name} src={poster} />}
{!poster && <div className={styles.spacer}></div>}
n3parikh commented 2021-07-12 17:56:52 -04:00
Owner
Copy Link

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.

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.
components/playground.tsx
@ -47,6 +51,9 @@ import { TeamMember } from "./TeamMember";
import { TeamMemberCard } from "./TeamMemberCard";
import { OrganizedContent, LinkProps } from "./OrganizedContent";
import { Button } from "./Button";
import { Footer } from "./Footer";
n3parikh commented 2021-07-12 17:54:50 -04:00
Owner
Copy Link

This import is unused.

This import is unused.
pages/_app.css Outdated
@ -18,6 +18,7 @@ body {
#1481e3 -17.95%,
#4ed4b2 172.82%
);
--background-teal-2: rgb(78, 212, 178, 0.2);
n3parikh commented 2021-07-12 17:53:47 -04:00
Owner
Copy Link

Nitpick: Can you move this to just below the --teal-2-20 line, and rename it to --teal-2-background, for consistency?

Nitpick: Can you move this to just below the `--teal-2-20` line, and rename it to `--teal-2-background`, for consistency?
👍 1
a3thakra reviewed 2021-07-18 19:41:24 -04:00
components/MiniTechTalkCard.module.css
@ -0,0 +1,38 @@
.card {
display: flex;
flex-direction: row;
max-width: calc(800rem / 16);
a3thakra commented 2021-07-18 19:41:23 -04:00
Copy Link

You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component

You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-18 19:43:25 -04:00
components/MiniTechTalkCard.module.css
@ -0,0 +25,4 @@
.content h1 {
margin-block-start: 4px;
margin: 0;
a3thakra commented 2021-07-18 19:43:24 -04:00
Copy Link

You can do:

margin: 0;
margin-top: calc(4rem / 16);

instead of margin-block-start and margin.

You can do: ```css margin: 0; margin-top: calc(4rem / 16); ``` instead of `margin-block-start` and `margin`.
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-19 00:22:43 -04:00
components/MiniTechTalkCard.module.css
@ -0,0 +5,4 @@
box-sizing: border-box;
padding: calc(16rem / 16);
color: var(--purple-2);
font-size: calc(14rem / 16);
a3thakra commented 2021-07-19 00:22:42 -04:00
Copy Link

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 :)

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 :)
a3thakra marked this conversation as resolved
a3thakra commented 2021-07-19 00:26:54 -04:00
Copy Link

Added a comment on figma re the mini tech talk card.

Added a comment on figma re the mini tech talk card.
a3thakra reviewed 2021-07-19 00:32:05 -04:00
components/TechTalkCard.tsx Outdated
@ -0,0 +12,4 @@
export function TechTalkCard({ name, poster, children }: TechTalkProps) {
return (
<article>
<section className={styles.card}>
a3thakra commented 2021-07-19 00:32:05 -04:00
Copy Link

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.

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.
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-19 00:33:03 -04:00
components/TechTalkCard.tsx Outdated
@ -0,0 +17,4 @@
{poster && <Image alt={name} src={poster} />}
{!poster && <div className={styles.spacer}></div>}
</aside>
<section className={styles.content}>
a3thakra commented 2021-07-19 00:33:03 -04:00
Copy Link

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. :)

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. :)
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-19 00:36:29 -04:00
components/TechTalkCard.module.css
@ -0,0 +3,4 @@
flex-direction: row;
max-width: calc(800rem / 16);
box-sizing: border-box;
padding: calc(24rem / 16);
a3thakra commented 2021-07-19 00:36:29 -04:00
Copy Link

I dont think you need this padding.

I dont think you need this padding.
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-19 00:37:28 -04:00
components/TechTalkCard.module.css Outdated
@ -0,0 +1,40 @@
.card {
display: flex;
flex-direction: row;
max-width: calc(800rem / 16);
a3thakra commented 2021-07-19 00:37:28 -04:00
Copy Link

You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component

You don't need to restrict the width here. This is automatically done on the pages. - using the DefaultLayout component
a3thakra marked this conversation as resolved
a3thakra reviewed 2021-07-19 00:39:36 -04:00
components/TechTalkCard.module.css Outdated
@ -0,0 +35,4 @@
flex-direction: column;
max-width: calc(1200rem / 16);
box-sizing: border-box;
padding: calc(24rem / 16);
a3thakra commented 2021-07-19 00:39:36 -04:00
Copy Link

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 :)

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 :)
c29wan added 3 commits 2021-07-22 18:12:28 -04:00
continuous-integration/drone/push Build is passing Details
8560a16ca1 Merge branch 'main' into feat/tech-talk-card
a3thakra reviewed 2021-07-22 21:24:20 -04:00
components/MiniTechTalkCard.tsx Outdated
@ -0,0 +12,4 @@
return (
<article className={styles.card}>
<aside>{poster && <Image alt={name} src={poster} />}</aside>
<section className={styles.content}>
a3thakra commented 2021-07-22 21:24:20 -04:00
Copy Link

a div over here, instead of a section would be preferred.

You should use sections inside an article only if articles are big enough.

a div over here, instead of a section would be preferred. You should use sections inside an article only if articles are big enough.
a3thakra reviewed 2021-07-22 21:30:13 -04:00
components/TechTalkCard.tsx Outdated
@ -0,0 +4,4 @@
interface TechTalkProps {
name: string;
short: string;
a3thakra commented 2021-07-22 21:30:13 -04:00
Copy Link

We don't need short inside a tech talk card.

We don't need short inside a tech talk card.
a3thakra commented 2021-07-23 17:17:44 -04:00
Copy Link

Only two nit-picks, good to go after that!!

Only two nit-picks, good to go after that!!
c29wan added 2 commits 2021-07-29 10:01:11 -04:00
continuous-integration/drone/push Build is passing Details
188896ac0b Merge branch 'main' into feat/tech-talk-card
a3thakra approved these changes 2021-07-29 15:43:20 -04:00
c29wan merged commit 1fb308327f into main 2021-07-29 16:56:48 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
a3thakra
Labels
Clear labels   
a11y

   
Backlog

   
Blocked

   
Bug

   
Content

   
Dependencies

   
Design

   
Feature Request

   
Good First Issue

   
In Progress

   
Performance

   
Priority - High

   
Priority - Low

   
Priority - Medium

   
Untriaged
No Label
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
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/www-new#71
No description provided.
Delete Branch "feat/tech-talk-card"

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?