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

feat: new account switcher #94

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

feat: new account switcher #94

wants to merge 3 commits into from

Conversation

meeh0w
Copy link
Member

@meeh0w meeh0w commented Nov 20, 2024 •
edited
Loading

⚠️ DO NOT MERGE, waiting for the balance rollup

Description

Changes

  • New account switcher designs
  • New account details view
  • No balance rollup yet (AC3), will be done in a separate PR

Testing

  • Play with the account switcher, try to break it

Screenshots:

New.Account.Switcher.mov

Checklist for the author

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

vvava
vvava requested changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@vvava vvava left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've been trying to test it as well and it seems it works well, love it

src/pages/Accounts/components/AccountsActionButton.tsx Outdated
}}
>
<Button
onClick={onAddNewAccount}
sx={{ gap: 1 }}
sx={{
mr: '1px !important',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid to use the important here and other places?

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't so easy with the current state of K2 - the other way I found was very verbose, so I went with !important. Let me take another look at this, I'll get back to you

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored in 9633fa8

@meeh0w meeh0w requested a review from vvava November 25, 2024 11:51
bferenc
bferenc approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@bferenc bferenc left a comment

Choose a reason for hiding this comment

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

works great! 👏

vvava
vvava approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@vvava vvava left a comment

Choose a reason for hiding this comment

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

thank you!

@meeh0w
Copy link
Member Author

meeh0w commented Nov 25, 2024

⚠️ DO NOT MERGE, waiting for the balance rollup

@meeh0w meeh0w added the DO NOT MERGE This PR is not meant to be merged in its current state label Nov 25, 2024
@meeh0w meeh0w mentioned this pull request Nov 27, 2024
Draft
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bferenc bferenc bferenc approved these changes

@vvava vvava vvava approved these changes

Assignees
No one assigned
Labels
DO NOT MERGE This PR is not meant to be merged in its current state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@meeh0w @vvava @bferenc