feat(ui): updates for address view #1941
Conversation
🦋 Changeset detectedLatest commit: 0055896 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 0055896): https://penumbra-ui-preview--pr1941-address-view-v2-iyhu8fit.web.app (expires Tue, 17 Dec 2024 20:23:10 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
packages/ui/src/Text/index.tsx
Outdated
| const truncateClass = truncate | ||
| ? cn('truncate max-w-[150px] overflow-hidden whitespace-nowrap') | ||
| : ''; // Adds max-width for aggressive truncation |
There was a problem hiding this comment.
what defines the truncation property's max width?
| </Text> | ||
| ) : ( | ||
| <Text technical truncate> | ||
| <Text technical truncate={truncate}> | ||
| {encodedAddress} | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
the copy button below has some stylistic inconsistencies. It displays different styles for sparse and compact densities, and it is also getting cut off. Should this be addressed at the button component level, or by adjusting the wrapping div tag to reduce its size?
There was a problem hiding this comment.
it should be fine in the real app, it's the Storybook that cuts the block. Could be solved by adding padding in the Storybook story around AddressView
There was a problem hiding this comment.
padding fixes it 👍
| return ( | ||
| <div className='flex items-center gap-2 text-text-primary'> | ||
| <div className={cn(getFont(density))}> |
There was a problem hiding this comment.
issue: this class doesn't apply to actual text. It applies to the wrapping div, but the Text component has more important styles that are applied. It would be best to apply the font classes using the props to Text component.
For example, in the image below Figma-generated CSS shows that we should use small prop of the Text for compact density.
<Text small truncate={truncate}>There was a problem hiding this comment.
makes sense, was questioning whether it only applied to wrapping div – will modify accordingly
| 'Sample opaque address view': ADDRESS_VIEW_OPAQUE, | ||
| 'Decoded: main-account address view': ADDRESS_VIEW_DECODED, | ||
| 'Decoded: sub-account address view': ADDRESS1_VIEW_DECODED, | ||
| 'Opaque: account address view': ADDRESS_VIEW_OPAQUE, |
There was a problem hiding this comment.
praise: thanks for covering all style cases in the docs 👍
packages/ui/src/Text/index.tsx
Outdated
| @@ -102,7 +102,9 @@ const getTextOptionClasses = ({ | |||
| break: breakProp, | |||
| whitespace, | |||
| }: TextProps): string => { | |||
| const truncateClass = truncate ? cn('truncate') : ''; | |||
| const truncateClass = truncate | |||
| ? cn('truncate max-w-[150px] overflow-hidden whitespace-nowrap') | |||
There was a problem hiding this comment.
issue: the max-w-[150px] class would break many of existing components. Truncation width is generally defined in the parent component.
also, truncate class applies both overflow-hidden and whitespace-nowrap under the hood
There was a problem hiding this comment.
I’ll move the truncation width to the parent component. Moving forward, I’ll focus on applying styling at the parent level where possible to avoid introducing breaking changes to existing components.
| <Text strong-bold truncate={truncate}> | ||
| {isRandomized && 'IBC Deposit Address for '} | ||
| {getAccountLabel(addressIndex.account)} | ||
| </Text> | ||
| ) : ( | ||
| <Text small truncate={truncate}> | ||
| {isRandomized && 'IBC Deposit Address for '} | ||
| {getAccountLabel(addressIndex.account)} | ||
| </Text> |
There was a problem hiding this comment.
thought: this makes me think of creating an alternative prop that would simply accept a string
aligns
Address ViewV2 UI component with the latest figma designs – review the storybook preview link. Some pointers would be helpful cc @VanishMax