-
Notifications
You must be signed in to change notification settings - Fork 2
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: redesign app #43
Conversation
- new theme - remove safe-react-components - update to MUI 5 - Use craco instead of react-rewired
src/components/ApprovalDialog.tsx
Outdated
<TextFieldInput | ||
> | ||
{changeApprovalItems.map((item) => ( | ||
<MenuItem value={item.id}>{item.label}</MenuItem> |
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.
<MenuItem value={item.id}>{item.label}</MenuItem> | |
<MenuItem key={item.id} value={item.id}>{item.label}</MenuItem> |
alt={tokenMap?.get(approval.tokenAddress)?.symbol} | ||
/> | ||
<div style={{ display: 'flex', flexDirection: 'column' }}> | ||
<Typography fontWeight={700}>{tokenMap?.get(approval.tokenAddress)?.symbol}</Typography> |
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.
Nit: if you move the symbol
getter outside of the render, you'd need not call this twice.
minHeight: 48, | ||
borderBottom: '2px solid #E8E7E6', | ||
width: 'inherit', | ||
borderBottom: `1px solid ${theme.palette.border.main}`, |
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.
You can remove reliance on useTheme
by using a callback here.
borderBottom: `1px solid ${theme.palette.border.main}`, | |
borderBottom: (theme) => `1px solid ${theme.palette.border.main}`, |
variant={uiStore.selectedApprovals.length === 0 ? 'outlined' : 'contained'} | ||
disabled={uiStore.selectedApprovals.length === 0} |
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.
Nit: you could create a hasSelectedApprovals
constant or similar to need not compare twice.
src/hooks/useDarkMode.ts
Outdated
import { useEffect, useState } from 'react'; | ||
|
||
const isSystemDarkMode = (): boolean => { | ||
if (typeof window === 'undefined' || !window.matchMedia) return false; |
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.
As you're not using NextJS, I don't think you need check for the existence of window
. You might be able to simplify this implementation.
src/hooks/useDarkMode.ts
Outdated
useEffect(() => { | ||
const isDark = isSystemDarkMode(); | ||
setIsDarkMode(isDark); | ||
document.documentElement.setAttribute('data-theme', isDark ? 'dark' : 'light'); |
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.
You aren't referencing this anywhere in your CSS at the moment. You can remove if if you want.
shortName: 'rin', | ||
currencySymbol: 'RIN', | ||
baseAPI: 'https://safe-transaction.rinkeby.gnosis.io', | ||
baseAPI: 'https://safe-transaction-mainnet.safe.global', |
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.
Suggestion: fetching this from the config service.
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.
I'd keep it out of scope for this PR.
console.log('Filtering approvals'); | ||
return logs.filter((log) => log.topics.length === 3); | ||
}) | ||
.then((logs) => | ||
logs.map((log) => ({ | ||
...log, | ||
tokenAddress: log.address, | ||
txHash: log.transactionHash, | ||
blockHash: log.blockHash, | ||
})), | ||
); | ||
|
||
return await Promise.all( | ||
approvalLogs.map(async (log) => ({ | ||
...log, | ||
timestamp: (await web3Provider.getBlock(log.blockHash)).timestamp, | ||
})), | ||
); | ||
console.log('Fetched approvalLogs'); |
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.
Not sure if you want to keep these logs?
Changes
Todos