feat-tech-join-team #269
feat-tech-join-team #269
Conversation
There was a problem hiding this comment.
praise: @Harshvardhan1012 looks great! Got a few nitpicks to do, and a bit of positioning between the text / image parts of this component in order to better match the design.
src/components/tech/JoinTeam.tsx
Outdated
| export const JoinTeam = () => { | ||
| return ( | ||
| <Section | ||
| mx={{ md: "94px", initial: "2" }} |
There was a problem hiding this comment.
nitpick: Always put the initial responsive size first so that it goes from smaller screens to larger screens when reading left-to-right. Check other parts of the code for this too.
src/components/tech/JoinTeam.tsx
Outdated
| align="center" | ||
| > | ||
| <Flex gap="2" direction="column" justify="center"> | ||
| <Heading size="8" className="text-navy-800"> |
There was a problem hiding this comment.
suggestion (blocking): Set the as parameter to 2 so that this creates an <h2> heading, which is important document structure for screenreaders and other accessibility technology.
src/components/tech/JoinTeam.tsx
Outdated
| <Heading size="8" className="text-navy-800"> | ||
| How To Join Our Team! | ||
| </Heading> | ||
| <Heading size="5"> |
There was a problem hiding this comment.
suggestion (blocking): This isn't really a heading, just emphasized text, so use the Strong component here or the Text component with appropriate font-weight.
src/components/tech/JoinTeam.tsx
Outdated
| </Text> | ||
|
|
||
| <Link href="mailto:[email protected]"> | ||
| <Button size="4" className="w-40"> |
There was a problem hiding this comment.
nitpick: Probably simpler to go with what Radix provides out of the box, and remove the width class. Alternatively, you could wrap the button in a Box component with a set width, and use the asChild property to pass that down to the button.
| px={{ md: "9", initial: "6" }} | ||
| py="6" | ||
| > | ||
| <Flex |
There was a problem hiding this comment.
suggestion (blocking): Based on the design, it looks like the text takes up a little more than half the space available. Can we try to use some flex properties to go for a 60 / 40 width for the text / image?
suggestion (blocking): Can we use the flex stretch properties to have the image fill the vertical space available, as per the design? Could add some vertical margin to the image if it's not supposed to stretch all the way to the top / bottom of the text.
Harshvardhan1012
force-pushed
the
feat/tech-jointeam
branch
from
258bc9a to
096b357
Compare
Harshvardhan1012
force-pushed
the
feat/tech-jointeam
branch
from
7c31686 to
78679cc
Compare
What changed?
Tech->Join Our team section
issue #264
How will this change be visible?