Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Running Lighthouse audit... |
danielsimao
force-pushed
the
feature/wallet-page
branch
2 times, most recently
from
d14814c to
4bd7f40
Compare
tomjeatt
left a comment
tomjeatt
left a comment
There was a problem hiding this comment.
Few comments, no blockers (considering it's your last day tomorrow!). Most of the comments relate to things we'll want to consider when we start using the patterns in this feature across the app, but while they're scoped to a single feature they can all be thought of as minor.
| type CTALinkProps = Props & NativeAttrs & InheritAttrs; | ||
| type InheritAttrs = Omit<BaseCTAProps, (keyof AriaAttrs & NativeAttrs & Props) | 'elementType'>; | ||
|
|
||
| type CTALinkProps = Props & NativeAttrs & AriaAttrs & InheritAttrs; |
There was a problem hiding this comment.
Nothing to change here immediately, but I'd like us to have a think about whether there's anything here that could be simplified. Looking at the before and after of the changes to this file, although the changes make sense in terms of utility, the component itself has become harder to follow.
There was a problem hiding this comment.
It's never easy to create these kind of components. What is the hardest part to follow about this component and what do you suggest simplifying?
There was a problem hiding this comment.
Nothing at the moment—as I said, I'd like to review it at a later date, when we have enough time to go through it properly.
| type WalletComponent = ForwardRefExoticComponent<IconProps & RefAttributes<SVGSVGElement>>; | ||
|
|
||
| const wallet: Record<string, WalletComponent> = { | ||
| 'polkadot-js': PolkadotJS, |
There was a problem hiding this comment.
why the js suffixes for polkadot and subwallet? Very very minor, but would be nice if we could lose it (Polkadot refer to it in their docs as the polkadot extension, so I don't think we need to include the js part).
There was a problem hiding this comment.
This is the name of the extension. There is not much I can do. https://github.com/TalismanSociety/talisman-connect/blob/master/packages/connect-wallets/src/lib/polkadotjs-wallet/index.ts
| xl: 1536 // large screen | ||
| }; | ||
|
|
||
| const breakpoints = { |
There was a problem hiding this comment.
I find this terminology confusing (up and down suggests vertical movement). I'd prefer some variation of max and min width: Better to keep our terminology as close to standard (i.e. css) as possible).
| @@ -1,3 +1,5 @@ | |||
| import { breakpoints } from '../utils/breakpoints'; | |||
There was a problem hiding this comment.
This can stay as it is for now, but I'd like to review it before we use it in any other places—the theme file is intended to be very simple, and this is moving away from that. Might be that we have to live with the complexity, but I'd prefer to explore other approaches first.
There was a problem hiding this comment.
I understand but it's also hard to reduce to 2-3 breakpoints. We need to realistic specially when it comes to small screens. We could reduce breakpoints that are above tablet, but under tablet, its where things get problematic on some screens
| @@ -0,0 +1,30 @@ | |||
| import { StyledMarginProps } from '../css/margin'; | |||
There was a problem hiding this comment.
Not sure about this—again, leave it for now as it's only being used in a single place, but I'd like to review this before using it more widely. Using a hook to return props (and therefore having props returned in the component body) isn't very intuitive. I like the intention—it's a good problem to solve—but would like to consider alternatives.
There was a problem hiding this comment.
The hook is necessary to map props to styled props. This hook will be used across the component library. I find myself a lot of times trying to apply some spacing and I would like to avoid creating a stylesheet just to add some margins here and there or even apply flex just so I could use gap. We should not create stylesheets for things that could be an existing feature, such as margin/padding.
| @@ -0,0 +1,24 @@ | |||
| import { CoinIcon, Flex, FlexProps, FontSize, IconSize, Span } from '@/component-library'; | |||
There was a problem hiding this comment.
I think there's a mismatch here between the component name and the sub-component names. DataGrid is very generic, but AssetCell, BalanceCell etc. are very specific. I don't think we need to overgeneralise, so I would suggest renaming DataGrid to something more specific (i.e., something which indicates what data is being shown).
There was a problem hiding this comment.
There are a variety of Cells repeated in all tables. There could be moved away from the DataGrid.
src/pages/Wallet/WalletOverview/components/AvailableAssetsTable/ActionsCell.tsx
Outdated
Show resolved
Hide resolved
| variant='text' | ||
| size='x-small' | ||
| > | ||
| <PencilSquare size='s' color='tertiary' /> |
There was a problem hiding this comment.
Not a code issue, but the pencil for switching accounts feels wrong—would ask Jay if there's a better icon for switch, e.g. like the icon polkadot use for switching between chains and parachains.
2 participants
Interbtc UI Pull Request Template
Description
Changes to
component-library:CTALink: addreact-ariauseLink+ allow external query parametersDivider: adddefaultvariant because we do not have avariantthat mimics our border style. Also addedsizefeature to allow thinner bordersFlex: addedmarginrelated props to allow:<Flex margin="spacing2"/>List: addedcardvariant (used in Wallet mobile)Switch: fix accessibility warningWalletIcon: new componentChange to
components:TableunderDataGrid.LoansBaseTableand made them reusable underDataGridDisclaimer: solely Wallet page is usingDataGrid.Change to
Loans:BorrowsPositionsTable+LendPositionsTableinto a single componentLoanPositionsTable.LoansBaseTablein favor of creating a generic approach this component that can be found inDataGrid/Table(same components, just better names and reusable)Changes to
AMM/Pools:PoolsTableAdded Wallet page
NOTE: Wallet page is fully tested