[TAS-303] Feat/ux feedback #53
Conversation
- review theme values - adapt component to use proper theme keys - update spacings - update component structure
- add main container margins - update History page WIP🚧 - limit apy values to after 06/06 - update fontwrights medium - theme consolidation WIP🚧 - add default address formatting
- remove transition on Popover, Menu and Dialog - rename text color to tertiary - use 4 decimals for balances - fix balances for AccountPopover - update right padding on History table
- add geo fence modal provider - add Dialog styles to theme WIP🚧 - move checkbox icons to theme lib - set default checkbox icons through theme
toniocodo
requested review from
dcodes05,
nick,
apexearth and
rafaelugolini
- update topnav padding - fix alignements - use text-ellipsis wherever possible - adapt inputMode for touch screens
- update fontSize and lineHeight to numerical values - update components for mobile devices - update theme with new typography variants
| mt: { xs: 3.25, md: 3 }, | ||
| mb: 10, | ||
| pt: (theme) => ({ | ||
| xs: '112px', |
There was a problem hiding this comment.
nit: seems like this one should be derived on math from the md value.
(i don't really care though)
There was a problem hiding this comment.
this padding accounts for the topnav height which has quite strange dimensions when < md: it wraps to 2 rows, the second one having distinct height than the first. I initially just wanted to do theme.mixins.toolbar.height * 2 but designs said otherwise 😅
| {intl.formatMessage(d.label)} | ||
| </Button> | ||
| ))} | ||
| </Stack> |
There was a problem hiding this comment.
The annoying thing about this versus tailwind is how 1 line of code is spread out into several lines of code.
<Stack
direction="row"
alignItems="center"
justifyContent="space-between"
>versus
<div className="flex items-center justify-between">Or
<Stack
direction="row"
sx={{
borderRadius: 1,
padding: 0.25,
backgroundColor: 'grey.700',
gap: 0.5,
}}
>versus
<div className="flex rounded p-1 bg-gray-700 gap-2">The difference is huge in this example.
There was a problem hiding this comment.
yeah tailwind is basically syntactic sugar for css while css-in-js engines are closer to original css. The good part is that you have type checking and autocompletion, I usually only type the first 2-3 letters and leave the rest to IDE. For some very repetitive, I even added some vs code user snippets
"intl-message": {
"prefix": "int",
"body": "intl.formatMessage({ defaultMessage: '' })",
"description": "react-intl default message"
},
"center": {
"prefix": "cen",
"body": "display=\"flex\" justifyContent=\"center\" alignItems=\"center\"",
"description": "flex center"
}
| fontFamily="Sailec" | ||
| color="primary.contrastText" | ||
| > | ||
| <Typography fontSize={24} fontWeight={700} fontFamily="Sailec"> |
There was a problem hiding this comment.
Is there a place for font families in the theme?
There was a problem hiding this comment.
the main ones are defined in theme.typography.fontFamily but it is not meant to be used "as-is". I have seen people adding other font families to the theme directly by adding custom props (i.e. theme.typography.monospace), but I think it's preferable to define text variants embedding the right font directly and just use font name for one-off customizations
| alignItems="center" | ||
| justifyContent="space-between" | ||
| px={{ xs: 2, md: 3 }} | ||
| py={{ xs: 1.5, md: 2 }} |
There was a problem hiding this comment.
I do like this style of defining breakpoints. It is sometimes easier to comprehend than when I read it in tailwind.
| reject(prop('isSelected')), | ||
| partition<TokenOption>(prop('isSwappable')), | ||
| )(tokens); | ||
| const toks = sortWith<TokenOption>([ |
| alpha(theme.palette.common.white, 0.1), | ||
| ':hover': { | ||
| background: (theme) => theme.palette.grey[600], | ||
| }, |
There was a problem hiding this comment.
Just for reference/example/info this would be replaced by the following approximation:
flex items-center justify-center rounded w-12 leading-2 text-secondary py-1 px-2 bg-white/10 hover:bg-gray-600
There was a problem hiding this comment.
yes, maybe just min-w-9 instead of w-12, leading-none instead of leading-2, py-0.5 and px-1
|
yeah, sorry for the huge PR but it embeds all comments made this last couple of days... it won't get that big every time 🤞 |
No description provided.