-
Notifications
You must be signed in to change notification settings - Fork 22
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(ui): #2053: ActionView
component
#2055
Conversation
Visit the preview URL for this PR (updated for commit 57d6e25): https://penumbra-ui-preview--pr2055-feat-2053-action-vi-9770fr20.web.app (expires Mon, 24 Feb 2025 07:49:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
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.
nice work! I really like the wrapper abstraction here for switching between opaque and visible views
|
||
return ( | ||
<ActionWrapper title='Output' opaque={value.outputView.case === 'opaque'}> | ||
{value.outputView.case === 'visible' && ( |
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.
ACK – conditional visibility rendering
<ActionWrapperHeader {...props} /> | ||
</div> | ||
|
||
{infoRows} |
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.
question: wondering if there's anything in this ActionWrapper
component or prop that might trigger unnecessary re-renders?
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'm a little worried about useDensity
calls – there are just too many of them within the ActionView
and its subcomponents. I'd like to test its performance later and maybe try to import this component dynamically in the DEX
spend: SpendAction, | ||
output: OutputAction, | ||
swap: SwapAction, | ||
// TODO: Implement the actions below |
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.
comment: reference https://github.com/penumbra-zone/web/blob/main/docs/adrs/003-action-views.md for details on which fields are marked as opaque or visible if you need more context.
Relates to #2053
This PR implements 3 action views: Spend, Output, and Swap. The rest actions are mocked and simply display 'Unimplemented'.
Check the component live here (change the 'action' parameter from the table): https://penumbra-ui-preview--pr2055-feat-2053-action-vi-9770fr20.web.app/?path=/docs/ui-library-actionview--docs