-
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
Implement the v2 assets/transactions pages using PenumbraUI #1607
Conversation
|
Visit the preview URL for this PR (updated for commit 207028b): https://penumbra-ui-preview--pr1607-jessepinho-assets-tr-ck5qw87m.web.app (expires Thu, 15 Aug 2024 17:51:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
501cf1f
to
b9f13a6
Compare
6012e7a
to
0d10fc4
Compare
607ef0e
to
8924ade
Compare
e9b2d2d
to
85b47a7
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.
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 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 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.
[v2PathPrefix(PagePath.TRANSACTIONS)]: <TransactionsCardTitle />, | ||
}; | ||
|
||
export const DashboardLayout = () => { |
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 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 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.
Copied almost verbatim from the original, except this one references the new <ValueViewComponent />
and adds the context='table'
prop
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.
Adapted/copied from https://github.com/penumbra-zone/web/blob/jessepinho/assets-transactions/apps/minifront/src/components/dashboard/assets-table/index.tsx, but with some changes to use new v2 components/etc.
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.
/** @todo: Remove next line after we switch to v2 layout */ | ||
str = str.replace('/v2', ''); |
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.
Needed this to make page path matching work with v2 paths
@@ -28,7 +35,7 @@ export interface CardProps { | |||
* ``` | |||
*/ | |||
as?: WebTarget; | |||
title?: string; | |||
title?: ReactNode; |
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.
Card titles can now be rich content
import * as RadixDialog from '@radix-ui/react-dialog'; | ||
import { Text } from '../Text'; | ||
import { Icon } from '../Icon'; | ||
import { X } from 'lucide-react'; | ||
import { ButtonGroup, ButtonGroupProps } from '../ButtonGroup'; | ||
|
||
const gradualBlur = (blur: string) => keyframes` |
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.
This animation was causing problems with the backdrop-filter
of the dialog background, so I nixed it for now. Apparently this is a known limitation of backdrop-filter
in Chrome.
@@ -66,6 +50,8 @@ const DialogContent = styled(RadixDialog.Content)` | |||
display: flex; | |||
flex-direction: column; | |||
gap: ${props => props.theme.spacing(6)}; | |||
|
|||
pointer-events: 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.
We had to add pointer-events: auto
here because we're using pointer-events: none
on DialogContent
above, so that clicks on DialogContent
are passed through to the Overlay
and thus close the dialog.
f030fc9
to
f998dc3
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.
5a85e8b
to
2e3af3c
Compare
<Grid tablet={8} desktop={6} xl={4}> | ||
<Card title={CARD_TITLE_BY_PATH[v2PathPrefix(pagePath)]}> | ||
<div className='flex flex-col gap-4'> | ||
<Tabs |
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.
issue (a11y, yet non-blocking): i noticed the Tabs are a little off with the designs:
- When hovered, there's no border-radius, the tabs are squared
- Using
tab
key on the assets pages doesn't show the outlines on some elements, so i didn't know where my focus is. It would be good to outline or apply hover styles on the focused tabs. tab
doesn't focus the "info" icon next to the "Asset balances" title. It more of the issue withButton iconOnly
component
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.
However, this is not exactly related to this page, so feel free to just not forget to fix the issues in other PRs :)
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.
- A more annoying issue. On the video, i use
tab
a couple of times. Firstly, there are no outline described in points 2-3, but then tab goes to the assets swap link. Notice how it takes twotab
s to focus out of the asset.
kqp.mp4
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 like the code structure and the how easy it it to read it. Only one blocking issue
This PR creates the first two v2 pages! I started with the simplest ones (assets/transactions) even though they're not 100% ready yet in Figma, because I wanted to just get something shipped.
Note that the layouts are a bit narrow at the moment. But I'm just following Figma designs, so please direct design feedback to Sam 😆 That said, these can be merged as a work in progress, because we're still building out the v2 layouts.
Screen.Recording.2024-08-05.at.9.54.18.PM.mov
In this PR
hexOpacity()
helper so I can stop makingTEN_PERCENT_OPACITY_IN_HEX
consts.<Display />
component that handles page max widths and horizontal margins.<Dialog />
<Dialog />
component should be based on a grid, just like the sizing of regular components on the page. So I had to rework the<Dialog />
component a bit so that the dialog is wrapped in a<Display />
and a<Grid />
and fits the specs in Figma.