-
Notifications
You must be signed in to change notification settings - Fork 15
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
Create initial pass of v2 send/receive pages #1665
Conversation
🦋 Changeset detectedLatest commit: 46dc416 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Visit the preview URL for this PR (updated for commit 46dc416): https://penumbra-ui-preview--pr1665-jessepinho-transfer-qsdpwkgt.web.app (expires Mon, 26 Aug 2024 11:55:55 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
37c22f8
to
c5f034e
Compare
97f1285
to
75ddb24
Compare
848012e
to
5aa3c78
Compare
eca6373
to
5593bbf
Compare
7c48de7
to
5ada544
Compare
5ada544
to
b6d68f2
Compare
import { GasFee } from '../../shared/gas-fee'; | ||
import { useBalancesResponses, useStakingTokenMetadata } from '../../../state/shared'; | ||
import { NonNativeFeeWarning } from '../../shared/non-native-fee-warning'; | ||
import { transferableBalancesResponsesSelector } from '../../../state/send/helpers'; | ||
import { useRefreshFee } from '../../v2/transfer-layout/send-page/use-refresh-fee'; |
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.
Note that I deleted the use-refresh-fee.ts
file from the v1 directory, and moved it to the v2 directory. Now, we're "reaching into" the v2 directory from v1 to import its logic.
I think this is a good approach for logic-heavy files (like hooks, etc.), since eventually, we'll want to just delete all the v1 code and move the v2 code up into src
. At that point, if we've already moved all the v1 hooks/etc. into the v2 directory, the migration will be a cinch.
And in the meantime, we don't need to maintain two copies of each hook file (i.e., we don't have to have a copy of use-refresh-fee.ts
in both the v1 src
directory, and the v2 directory).
@@ -6,7 +6,7 @@ import { ReactNode, useCallback, useEffect, useState } from 'react'; | |||
import { | |||
getAddressIndex, | |||
getAmount, | |||
getAssetIdFromBalancesResponseOptional, | |||
getAssetIdFromBalancesResponse, |
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 got rid of the optional one, since we can now just call .optional()
on it.
</Dialog> | ||
</div> | ||
); | ||
export const AssetsCardTitle = () => { |
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 may want to review this file with whitespace changes hidden. I just added a useId()
call, and then passed it to the dialog trigger and content to make the dialog "grow out of" the info icon.
</Dialog> | ||
</div> | ||
); | ||
export const TransactionsCardTitle = () => { |
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 may want to review this file with whitespace changes hidden. I just added a useId()
call, and then passed it to the dialog trigger and content to make the dialog "grow out of" the info icon.
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.
Changes in this file relate to transitions between the assets and transactions pages, and use the useAnimationDeferredValue
hook (docs).
.optional() | ||
.pipe(getMetadata) | ||
.pipe(getAssetId); | ||
export const getAssetIdFromBalancesResponse = getBalanceView.pipe(getMetadata).pipe(getAssetId); |
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 can now just call .optional()
on this, so no need to hard-code an optional version.
import { PenumbraUIProvider } from '../src/PenumbraUIProvider'; | ||
import { Density } from '../src/Density'; | ||
import { Tabs } from '../src/Tabs'; | ||
import styled from 'styled-components'; | ||
|
||
const WhiteTextWrapper = styled.div` |
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.
All <Text />
components now render with text.primary
color by default, so this is no longer needed.
@@ -21,6 +21,7 @@ | |||
"./lib/utils": "./lib/utils.ts", | |||
"./postcss.config.js": "./postcss.config.js", | |||
"./styles/*": "./styles/*", | |||
"./hooks/*": "./src/hooks/*/index.ts", |
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.
So that consumers can import our hooks
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.
Added a tooltip to the IBC Deposit toggle
const BorderWrapper = styled.div` | ||
border-radius: ${props => props.theme.borderRadius.full}; | ||
border: 1px solid ${props => props.theme.color.other.tonalStroke}; | ||
overflow: hidden; | ||
`; | ||
|
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.
There should now be a border on <AssetIcon />
, per latest Figma designs.
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.
Moved the entire <AssetIcon />
component up to the root src
directory, since it's now used by multiple other components in Penumbra UI (and can be used by consumers).
|
||
const iconOnlyAdornment = css<StyledButtonProps>` | ||
border-radius: ${props => props.theme.borderRadius.full}; | ||
padding: ${props => props.theme.spacing(1)}; | ||
width: max-content; |
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.
Fixes a weird issue where the button was sometimes full-width when it shouldn't have been
@@ -28,6 +31,7 @@ const compact = css<StyledButtonProps>` | |||
padding-right: ${props => props.theme.spacing(props.$iconOnly ? 2 : 4)}; | |||
height: 32px; | |||
min-width: 32px; | |||
width: max-content; |
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.
Fixes a weird issue where the button was sometimes full-width when it shouldn't have been
@@ -57,7 +61,7 @@ interface StyledButtonProps { | |||
$getBorderRadius: (theme: DefaultTheme) => string; | |||
} | |||
|
|||
const StyledButton = styled.button<StyledButtonProps>` | |||
const StyledButton = styled(motion.button)<StyledButtonProps>` |
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.
Enabling framer motion on the button
const WhiteTextWrapper = styled.div` | ||
color: ${props => props.theme.color.text.primary}; | ||
`; |
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.
WhiteTextWrapper
is required for this story, since <CharacterTransition />
is unstyled text, and I didn't want to wrap this story in a <Text />
or else consumers might think that it was styled out of the box.
@@ -11,10 +11,10 @@ const Root = styled.label` | |||
`; | |||
|
|||
const HelperText = styled.div<{ $disabled: boolean }>` | |||
${small} |
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.
Moving this to before color
, since small
now has its own color styles
@@ -20,6 +20,7 @@ const Root = styled.span<{ $density: Density; $priority: Priority }>` | |||
display: inline-block; | |||
max-width: 100%; | |||
width: max-content; |
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.
Prevents a weird issue where the pill is sometimes full-width
/** Content that will appear above the table. */ | ||
title?: ReactNode; | ||
children: ReactNode; | ||
/** Which CSS `table-layout` property to use. */ | ||
layout?: 'fixed' | 'auto'; | ||
tableLayout?: 'fixed' | 'auto'; |
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.
Renamed this prop to avoid confusion with framer-motion's layout
prop
# Conflicts: # packages/ui/src/Button/index.tsx
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.
@grod220 I am going to merge this PR and continue the work over this code. Any objections?
This is how the PR looks in action:
kap.mp4
The future work includes adding the AssetSelector and moving some more logic here
Go for it! 👍 |
First of all, sorry for the huge PR 😭 I was working on a lot of moving parts at once, and they're all a bit intertwined, and I wasn't able to separate them out in time.
Also, a lot of the diffs in this PR involve whitespace changes due to nesting, etc. So I'd recommend reviewing it with whitespace changes hidden.
In this PR
useAnimationDeferredValue()
hook that prevents glitches in an animation if data loads while the animation is still in progress.layout
andlayoutId
props to instead accept amotion
prop that can accept an object of framer-motion props (docs).<SegmentedControl />
to allow arbitrary value types, not just string values. This makes the component much more flexible/powerful.color
to be set directly on both the<Text />
and<Icon />
components via a callback — e.g.,<Text color={color => color.text.primary}>...</Text>
detailTechnical
text variant.<AssetSelector />
component. Not 100% finished yet, but can still be merged and iterated on.<Tooltip />
component.motion
prop to<Table />
and<Card />
.<Dialog />
out of its trigger (could use some de-janking):Screen.Recording.2024-08-16.at.8.21.34.PM.mov