Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat-tech-working-section #268

Open
wants to merge 1 commit into
base: saga
Choose a base branch
from
Open

feat-tech-working-section #268

wants to merge 1 commit into from

Conversation

Harshvardhan1012
Copy link
Contributor

What changed?

Working Section in Tech Page
Issue #258

How will this change be visible?

working-1

working-2

Copy link
Contributor

@jtfairbank jtfairbank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise: @Harshvardhan1012 pretty much there on the first shot! Just a few quick touchups and I can get this merged.

gap="4"
align="start"
direction="column"
p="20px"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): Should we just use Radix spacing size 4 (16px) or size 5 (24px) here, instead of a specific px value?

p="20px"
width="full"
justify="start"
maxWidth={{ md: "610px", initial: "auto" }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Would it be more maintainable to wrap this <Flex> in a <Container> to set the max width? Size 2 containers have a max width of 688px.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): Always put the initial attribute first, so that we can understand the sizes as they grow larger from left to right. Please review other changes in this PR to ensure they also follow this code convention. :)

<Badge
key={index}
className={`${
index % Object.keys(colors).length === 0 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: It'd be helpful to include a comment here to clarify the intent of this expression. It seems like you're choosing to make the badge text color light or dark based on the background color?

: "text-black"
}`}
style={{
backgroundColor: colors[index % Object.keys(colors).length],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): Good to leave a comment here documenting what this expression does (i.e. cycles through the colors based on label index).


export const WorkingSection = () => {
return (
<Section my="9" mx={{ initial: "5", md: "94px" }} p="0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Would it be clearer to have a <Section> wrap a <Container> to set the appropriate width and auto-center the content?

return (
<Section my="9" mx={{ initial: "5", md: "94px" }} p="0">
<Flex gap="4" direction="column" justify="center" align="center">
<Heading align="center" className="text-navy-900 text-5xl">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): Would set the font size through Radix'ssize propertyon Heading & other typeography elements. Just choose the nearest size to the design!

{title}
</Text>
<Text size="4" className="leading-10">
{para}.<Link href={`https://${link}`}>[GitHub Repo]</Link>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Include the period in the para text so it holds full sentences. And then include a space between that and the GitHub Repo link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create "What we are working on" section of the Tech page
2 participants