-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DSDK-176][FEAT][SMPL] Create Home skeleton #10
Conversation
@aussedatlo is attempting to deploy a commit to the LedgerHQ Team on Vercel. To accomplish this, @aussedatlo needs to request access to the Team. Afterwards, an owner of the Team is required to accept their membership request. If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account. |
166e61e
to
9b850b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean π
padding-left: ${({ theme }: { theme: DefaultTheme }) => theme.space[5]}px; | ||
`; | ||
|
||
const MenuTitle = styled(Box)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] consider using a Text
component instead, with variant
and fontWeight
props.
cf. https://react-ui-storybook.vercel.app/?path=/docs/asorted-typography-text--docs
apps/sample/src/pages/index.tsx
Outdated
color: ${({ theme }: { theme: DefaultTheme }) => theme.colors.neutral.c90}; | ||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] you can use a Flex
component from react-ui for that. Applies to all other places where you used Box
with display: flex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good but I think that you should leverage much more of react-ui's potential by using components such as Text
(with props like variant
and fontWeight
) and Flex
. There are a bunch of examples in LLD and LLM if you want.
Using text variants specifically is important as it helps us to remain consistent with the design system which defines a set of variants which have their specific font size & family.
Before implementing any design, in case the variant & font weight are not clearly displayed in design files, it should be reported to the designer. Same for unorthodox spacing values which are not in the defined ranges.
9b850b6
to
f8b8b30
Compare
e3b201c
to
9aff145
Compare
f8b8b30
to
6c110d5
Compare
3b24209
to
aa845e1
Compare
6c110d5
to
f3dd1d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[should] import from @ledgerhq/react-ui
instead of @ledgerhq/react-ui/index
f3dd1d9
to
e96a60d
Compare
variant: "paragraph", | ||
fontWeight: "semiBold", | ||
})` | ||
margin-left: ${({ theme }: { theme: DefaultTheme }) => theme.space[5]}px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI spacing also works nicely with styled system: you can add a prop marginLeft: 5
or ml: 5
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ml: 5
looks cleaner and more concise than ${({ theme }: { theme: DefaultTheme }) => theme.space[5]}px
. I'll edit all the spacing to use styled-system
π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π
padding: ${({ theme }: { theme: DefaultTheme }) => theme.space[6]}px; | ||
background: ${({ theme }: { theme: DefaultTheme }) => | ||
theme.colors.neutral.c30}; | ||
border-radius: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ASK] can border-radius: 8px
be instead derived from theme.radii[2] ?
cursor: pointer; | ||
align-items: center; | ||
opacity: 0.7; | ||
padding: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] padding to be taken from theme.space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice ! thanks for figuring out the fonts setup !
e96a60d
to
fa2efbb
Compare
fa2efbb
to
d101129
Compare
π Description
Create Home skeleton from figma
β Context
β Checklist
Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.
npx changeset
was attached.[ ] Impact of the changes:
π§ Checklist for the PR Reviewers