-
Notifications
You must be signed in to change notification settings - Fork 188
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: desaturate & opacity for unapproved tokens #52
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -43,7 +43,7 @@ function TokenImg({ token, ...rest }: TokenImgProps) { | |||
return <img src={src} alt={alt} key={alt} onError={onError} {...rest} /> | |||
} | |||
|
|||
export default styled(TokenImg)<{ size?: number }>` | |||
export default styled(TokenImg)<{ size?: number; unapproved?: boolean }>` |
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.
call this approved
do the name doesn't include a boolean modifier
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.
chose to use unapproved
so that the default tokenImg isn't unapproved and shows in full saturation; you'd have to explicitly set a tokenImg as unapproved
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.
If I'm reading the code correctly, it looks like approvalState
is this enum:
export enum ApprovalState {
UNKNOWN = 'UNKNOWN',
NOT_APPROVED = 'NOT_APPROVED',
PENDING = 'PENDING',
APPROVED = 'APPROVED',
}
Would you not get the desired behavior if you just pass down approval state to the style, then make the decision about saturation there?
like:
${({ approvalState }) => approvalState === ApprovalState.NOT_APPROVED && 'filter: grayscale(100%) opacity(60%)'}
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 is already done in https://github.com/Uniswap/widgets/blob/main/src/components/Swap/Input.tsx#L38-L41.
Is this meant to replace that? Could this instead be done by copying or re-using the Input.tsx
code for Output.tsx
?
@@ -52,4 +52,6 @@ export default styled(TokenImg)<{ size?: number }>` | |||
border-radius: 100%; | |||
height: ${({ size }) => size || 1}em; | |||
width: ${({ size }) => size || 1}em; | |||
|
|||
${({ unapproved }) => unapproved && 'filter: grayscale(100%) opacity(60%)'} |
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.
use 'saturate(0) opacity(0.4)'
instead (I may have the opacity value wrong, but this is what is already in the code). Desaturation is the stated goal, so saturate
is more semantic than grayscale
.
onClick: () => void | ||
} | ||
|
||
export default function TokenButton({ value, collapsed, disabled, onClick }: TokenButtonProps) { | ||
export default function TokenButton({ value, collapsed, disabled, isUnapprovedToken, onClick }: TokenButtonProps) { |
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.
Same - use approved: boolean
or approval: ApprovalState
.
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.
Whatever you use here should be passed directly to TokenImg
.
If token has not yet been approved, desaturate & change opacity of the token logo:
Unapproved:
Approved: