Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kristiehuang
Copy link
Contributor

@kristiehuang kristiehuang commented Jul 5, 2022

If token has not yet been approved, desaturate & change opacity of the token logo:

Unapproved:
Screen Shot 2022-07-05 at 3 19 07 PM
Screen Shot 2022-07-05

Approved:
Screen Shot 2022-07-05 at 3 19 14 PM

@vercel
Copy link

vercel bot commented Jul 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
widgets ✅ Ready (Inspect) Visit Preview Jul 6, 2022 at 4:05PM (UTC)

@vercel vercel bot temporarily deployed to Preview July 5, 2022 19:22 Inactive
@@ -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 }>`
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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%)'}

@vercel vercel bot temporarily deployed to Preview July 6, 2022 16:05 Inactive
Copy link
Contributor

@zzmp zzmp left a 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%)'}
Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor

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.

@kristiehuang kristiehuang marked this pull request as draft July 22, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants