Skip to content

Conversation

@georgeweiler
Copy link
Contributor

@georgeweiler georgeweiler commented Jan 21, 2022

@github-actions
Copy link

Comment on lines +24 to +25
fontSize="60px"
lineHeight="64px"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should use relative units not fixed eg. px. Eg. the screen size changes, the text still has 60px but should be smaller. So we should probably use the object syntax or the array syntax to provide responsive styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but we don't have designs for how each typography element should adjust at different breakpoints. I think we should ticket this out for later. We can have Liz and Sasha come up with the sizes they want and add the responsive values in a future PR. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines -50 to +60
<Body2 paddingRight={8}>{heroText}</Body2>
<>
<Body2>{heroText1}</Body2>
<Body2>{heroText2}</Body2>
</>
) : (
<H4 maxW="500px">{heroText}</H4>
<Stack maxW="450px" justify="center">
<H4>{heroText1}</H4>
<H4 noOfLines={2}>{heroText2}</H4>
</Stack>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we set the responsive styles correctly (please see comment above) we will avoid conditional rendering for mobile.

Comment on lines 48 to 63
{isLarge ? (
<H3 {...restProps}>
{shouldRenderTokenAmount ? _tokenAmount : "--"}
</H3>
) : (
<H5 {...restProps}>
{shouldRenderTokenAmount ? _tokenAmount : "--"}
</H5>
)}
{withSymbol &&
tokenSymbol &&
(isLarge ? (
<Body1 alignSelf="center">{tokenSymbol}</Body1>
) : (
<Body3 alignSelf="center">{tokenSymbol}</Body3>
))}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

{/* exchange rate link */}
<HStack justify="center" mt={4}>
<ExternalLink
as="p"
Copy link
Contributor

Choose a reason for hiding this comment

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

why as="p"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I think this is just a copy/paste issue. I've updated.

@Battenfield Battenfield merged commit e9b15a5 into main Jan 24, 2022
@github-actions
Copy 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.

4 participants