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.

Sign up for GitHub

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

Jump to bottom

Feature/balance input #372

Merged
merged 14 commits into from
Jul 11, 2022
Merged

Feature/balance input #372

merged 14 commits into from
Jul 11, 2022

Conversation

crypto-engineer
Copy link
Contributor

@crypto-engineer crypto-engineer commented Jul 7, 2022 •
edited
Loading

Capture

@crypto-engineer crypto-engineer added the enhancement New feature or request label Jul 7, 2022
@crypto-engineer crypto-engineer self-assigned this Jul 7, 2022
@vercel
Copy link

vercel bot commented Jul 7, 2022 •
edited
Loading

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

Name Status Preview Updated
interbtc-ui-interlay ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 1:48PM (UTC)
interbtc-ui-interlay-testnet ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 1:48PM (UTC)
interbtc-ui-kintsugi ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 1:48PM (UTC)
interbtc-ui-kintsugi-testnet ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 1:48PM (UTC)

@crypto-engineer crypto-engineer marked this pull request as draft July 7, 2022 08:17
@github-actions
Copy link

github-actions bot commented Jul 7, 2022

Running Lighthouse audit...

@crypto-engineer
Copy link
Contributor Author

@crypto-engineer I don't think we need separate components for TokenField and TokenFieldWithBalance as it's only a very minor UI change. Instead, I'd be happier with a single TokenField component which takes an optional balance property.

Actually, I planned for granular components and composition with them since in cases of TokenField without TokenBalance, unused code could be shipped to users.

OK, I will update as you suggest.

@tomjeatt
Copy link
Collaborator

tomjeatt commented Jul 8, 2022

@crypto-engineer I don't think we need separate components for TokenField and TokenFieldWithBalance as it's only a very minor UI change. Instead, I'd be happier with a single TokenField component which takes an optional balance property.

Actually, I planned for granular components and composition with them since in cases of TokenField without TokenBalance, unused code could be shipped to users.

OK, I will update as you suggest.

Great, thanks Anton

@crypto-engineer
Copy link
Contributor Author

@tomjeatt
Could you please review it again? Thanks.

peterslany
peterslany reviewed Jul 8, 2022
View reviewed changes
Copy link
Contributor

@peterslany peterslany left a comment

Choose a reason for hiding this comment

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

Hello @crypto-engineer, thanks for the changes and good job with the PR :) I left just one small comment about the USD value of balance in TokenField component.

src/component-library/TokenField/TokenField.tsx Outdated Show resolved Hide resolved
tomjeatt
tomjeatt requested changes Jul 11, 2022
View reviewed changes
Copy link
Collaborator

@tomjeatt tomjeatt left a comment

Choose a reason for hiding this comment

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

Hi @crypto-engineer - looks good. One small change, but other than that good to go.

src/component-library/TokenField/TokenField.style.tsx Outdated Show resolved Hide resolved
tomjeatt
tomjeatt approved these changes Jul 11, 2022
View reviewed changes
Copy link
Collaborator

@tomjeatt tomjeatt left a comment

Choose a reason for hiding this comment

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

Looks good @crypto-engineer – I'm not sure whether we want to throw errors in a UI component, but it's a minor question and something we can talk about at some point in the next couple of weeks.

@crypto-engineer crypto-engineer merged commit 31f79fd into interlay:master Jul 11, 2022
@crypto-engineer crypto-engineer deleted the feature/balance-input branch July 11, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterslany peterslany peterslany left review comments

@tomjeatt tomjeatt tomjeatt approved these changes

Assignees

@crypto-engineer crypto-engineer

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@crypto-engineer @tomjeatt @peterslany