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

Add tokens section #1467

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add tokens section #1467

wants to merge 5 commits into from

Conversation

OKendigelyan
Copy link
Contributor

@OKendigelyan OKendigelyan commented Jul 8, 2024

Proposed changes

Task link

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

Screenshots

Add the screenshots of how the app used to look like and how it looks now

Desktop Mobile
image image

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

@OKendigelyan OKendigelyan force-pushed the add-tokens-section branch 2 times, most recently from 6d7c77a to f09dcab Compare July 8, 2024 11:44
@OKendigelyan OKendigelyan marked this pull request as ready for review July 8, 2024 11:45
Copy link
Contributor

@serjonya-trili serjonya-trili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the screenshots

import { useColor } from "../../styles/useColor";

// Known verified tokens on mainnet
const verifiedTokens = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth moving to umami/tezos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

@@ -11,29 +12,24 @@ body {
}

@media (max-width: 768px) {
padding: 0;
padding: 0 !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was wrong with the previous approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modal overlay adds additional padding to the body and it breaks the layout

@@ -200,6 +262,7 @@ const theme = extendTheme({
},
},
":root": {
"--chakra-colors-black": mode(dark.grey.white, light.grey.white)(props),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? black is white?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black is white in the light mode and vice versa 🤪

import repeat from "lodash/repeat";

// TODO: move this to utils
const getSmallestUnit = (decimals: number): string => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it can be moved to umami/core

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

tokenDecimals,
tokenSymbolSafe,
} from "@umami/core";
import repeat from "lodash/repeat";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we prohibit default imports somewhere 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why do we prohibited default imports from 3rd party libs?

@@ -26,14 +28,9 @@ export const AccountSelector = () => {
</Text>
<Flex alignItems="center" gap="4px">
<Text color={color("700")} size="sm">
{accounts[0].label}
{formatPkh(accounts[0].address.pkh)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why so? IIRC we should show both the label and the address

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to design there should be only account address
image

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.

None yet

2 participants