-
Notifications
You must be signed in to change notification settings - Fork 10
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
UI redesign part 4 #883
base: UI-redesign-part3
Are you sure you want to change the base?
UI redesign part 4 #883
Conversation
|
To be well tested.
a332b16
to
4ab610b
Compare
I use transparentModal as a presentation mode..!
However, scroll down to close modal is not working on Android
d6f286e
to
d0f26b0
Compare
2699872
to
b790895
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.
Just a quick pass over the diff!
I did not do any functional testing since it's still in draft mode.
|
||
export default RoundedCard | ||
|
||
const RoundedCardContainer = styled.View` |
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.
The pattern we've used so far is RoundedCardStyled
for this sort of components
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.
Done
@@ -30,13 +30,14 @@ interface FooterMenuProps extends BottomTabBarProps { | |||
style?: StyleProp<ViewStyle> | |||
} | |||
|
|||
const isIos = Platform.OS === 'ios' |
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: we have this in other places too. Let's extract it to a util somewhere or just use Platform.OS === 'ios'
which is short enough imo
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.
Done
useEffect(() => { | ||
const fetchCopiedText = async () => { | ||
const text = await getStringAsync() | ||
setCopiedText(text) | ||
} | ||
|
||
fetchCopiedText() | ||
}) |
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.
The dependency array is missing.
Also, I started preferring this pattern, WDYT?
useEffect(() => { | |
const fetchCopiedText = async () => { | |
const text = await getStringAsync() | |
setCopiedText(text) | |
} | |
fetchCopiedText() | |
}) | |
useEffect(() => { | |
getStringAsync().then(setCopiedText) | |
}, []) |
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 also like it better I think.
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.
Changed to the pattern you proposed
@@ -97,6 +115,10 @@ const Input = <T extends InputValue>({ | |||
onBlur && onBlur(e) | |||
} |
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.
Missing dependency array
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.
Code has been removed
@@ -55,7 +61,8 @@ import { SessionRequestData } from '~/types/walletConnect' | |||
import { showExceptionToast } from '~/utils/layout' | |||
import { getTransactionAssetAmounts } from '~/utils/transactions' | |||
|
|||
interface WalletConnectSessionRequestModalProps<T extends SessionRequestData> extends ModalContentProps { | |||
interface WalletConnectSessionRequestModalProps<T extends SessionRequestData> { | |||
walletConnectClient: SignClient |
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.
Isn't this accessible via useWalletConnectContext
?
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.
what a diff...
Have you tested all possible WC methods by using the example dapp?
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.
It's just code blocks moving around. No code changes.
import { DEFAULT_MARGIN } from '~/style/globalStyle' | ||
|
||
interface BottomModalHeaderProps { | ||
handleClose: () => void |
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.
we tend to call these props on...
not handle...
6e867be
to
2d6d313
Compare
Goal: reduce visual load to a minimum
3f80a98
to
ed21fc6
Compare
Compute bottom buttons height automatically, adjust gradient and list paddingBottom accordingly. Tweak pointerEvents.
No description provided.