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: set mock wallet address via UI #1062

Closed

Conversation

MatthewCYLau
Copy link
Contributor

@MatthewCYLau MatthewCYLau commented Aug 26, 2022

Address #578

What has been done

  • Web3Provider, expose function to set mock wallet address i.e. setMockWalletAddress

  • Inside of WalletSelector, added input to allow for setting mock wallet address i.e. invoke setInputMockWalletAddress
    image

  • Note, the Watch Address button is only enabled when input contains a valid address (via utils.isAddress)
    image

  • Render relevant visual cues in WalletWidget if mockAddress is present in useWeb3Context. Added new buttons to Switch Wallet, and Disconnect; removed the old Disconnect list item

  • When user clicks Switch Wallet, render wallet selector modal and hide wallet widget
    image

  • In Borrow modal, render warning text of mock wallet address is present
    image

  • Disable transaction button if mock address is present

  • Remove "Switch Network" error from modals if mock address is present

@0x4Graham
Copy link
Contributor

0x4Graham commented Aug 26, 2022

--Edit---

I see that this is still listed as one of the the to-do's

gm Matthew. This is great!

Just one behaviour that should not happen. When I enter a value in a text field such as withdraw, the buton should stay disabled.

See the behaviour that is a bug:

Screen.Recording.2022-08-26.at.13.22.52.mov

@iamanastasia
Copy link
Contributor

  1. Can we validate wallet address? Seems I can enter any text right now, and app will allow me to 'watch'.

Screenshot 2022-08-26 at 16 59 43

  1. Would love to see 2 buttons in profile dropdown — Switch account and Disconnect. Switching account saves 1 click if I want to exit watching mode and re-connect with a wallet. Here is the design:

Screenshot 2022-08-26 at 17 09 13

  1. Also we probably need to remove 'Wrong network' message. Since user doesn'y care much about it, and there is no way for him to switch network.

Screenshot 2022-08-26 at 17 00 34

@defispartan defispartan linked an issue Aug 26, 2022 that may be closed by this pull request
5 tasks
@github-actions
Copy link

github-actions bot commented Aug 29, 2022

✅ CI run has succeded!
You can find tests and deployment logs at https://github.com/aave/interface/actions/runs/2949079449'

@github-actions
Copy link

github-actions bot commented Aug 29, 2022

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 506.87 KB (🟡 +1.3 KB)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Eight Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/ 39.33 KB (🟡 +1 B) 546.2 KB
/faucet 21.86 KB (🟡 +1 B) 528.72 KB
/governance 63.48 KB (🟡 +13 B) 570.35 KB
/governance/ipfs-preview 104.28 KB (🟡 +26 B) 611.15 KB
/governance/proposal 104.44 KB (🟡 +25 B) 611.3 KB
/governance/proposal/[proposalId] 59.2 KB (🟡 +25 B) 566.07 KB
/reserve-overview 60.57 KB (-1 B) 567.43 KB
/staking 32.33 KB (🟡 +59 B) 539.19 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@github-actions
Copy link

@iamanastasia
Copy link
Contributor

@MatthewCYLau thank you so much! everything looks amazing

@grothem
Copy link
Collaborator

grothem commented Sep 22, 2022

@MatthewCYLau could you merge in latest main when you get a chance

@defispartan
Copy link
Collaborator

@MatthewCYLau Looks like there is one unused variable in WalletWidget that the ci is complaining about

@MatthewCYLau
Copy link
Contributor Author

MatthewCYLau commented Sep 22, 2022

👋 @grothem @defispartan feature branch has been updated with latest main plus removed the un-used import inside of WalletWidget

@iamanastasia could I get some steering on a styling edge case, that is mobile dark mode, the background colour for Switch Wallet, and Disconnect CTAs? 👇 Currently, they blend to the dark mode background. Should we go for an even darker shade of grey for these CTAs their background colour?

image

@iamanastasia
Copy link
Contributor

@MatthewCYLau yeah, that would be great. I think, that's because:

  1. Menu background color doesn't follow design (not your fault, just legacy).
  2. Buttons should have an outlined border (like button 'Details' in the dashboard in dark mode).

So for the background (both top bar and layout) let's change color to #1B2030 (Probably there is a style called 'header' in the dark theme). Would be great, if we can add little transparency 98% (optional).
Current app:
Screenshot 2022-09-23 at 13 51 23
New color for the backround:
Screenshot 2022-09-23 at 13 57 48

As for the buttons, reference would be 'Details' button in dark mode. Same style, size L. Border: 1px solid rgba(235, 235, 237, 0.12) (style is called 'Outlined Resting Border').
Screenshot 2022-09-23 at 13 56 39

I updated Figma file as well.

@MatthewCYLau
Copy link
Contributor Author

MatthewCYLau commented Sep 24, 2022

Hi 👋 @iamanastasia thank you for the steering 🙏 I've updated the styling based on your feedback. Screenshot below is the latest mobile menu in dark mode. Note the key changes that are top bar, and layout background colour plus buttons.

image

src/components/WalletConnection/WalletSelector.tsx Outdated Show resolved Hide resolved
src/components/infoTooltips/WatchOnlyModeTooltip.tsx Outdated Show resolved Hide resolved
src/components/WalletConnection/WalletSelector.tsx Outdated Show resolved Hide resolved
src/components/WalletConnection/WalletSelector.tsx Outdated Show resolved Hide resolved
@@ -94,7 +94,7 @@ export function AppHeader() {
top: 0,
transition: theme.transitions.create('top'),
zIndex: theme.zIndex.appBar,
bgcolor: 'background.header',
bgcolor: '#1B2030',
Copy link
Contributor

@drewcook drewcook Sep 26, 2022

Choose a reason for hiding this comment

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

@MatthewCYLau - Instead of us updating this for this singular spot, I believe we need to update the background.header value in theme.tsx instead.

@iamanastasia - Do these color updates need to get applied to several areas also? Because updating out theme is the right answer to keep things consistent and easy to scale. I think you'd need to vet what gets affected design-wise with this.

src/layouts/WalletWidget.tsx Show resolved Hide resolved
src/layouts/WalletWidget.tsx Show resolved Hide resolved
@@ -18,7 +18,7 @@ export const DrawerWrapper = ({ open, setOpen, children, headerHeight }: DrawerW
sx={{ top: `${headerHeight}px` }}
PaperProps={{
sx: {
background: 'rgba(56, 61, 81, 0.88)',
background: 'rgba(27, 32, 48, 0.98)',
Copy link
Contributor

@drewcook drewcook Sep 26, 2022

Choose a reason for hiding this comment

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

@MatthewCYLau @iamanastasia Here as well, See previous comment #1062 (comment) - i.e. what else gets these new color updates in the UI?

src/libs/web3-data-provider/Web3Provider.tsx Outdated Show resolved Hide resolved
@drewcook
Copy link
Contributor

@MatthewCYLau I will be breaking out all these code style changes into a separate PR and will take care of them and merge into this branch

@github-actions
Copy link

github-actions bot commented Sep 27, 2022

❌ CI run has failed!
Please see logs at https://github.com/aave/interface/actions/runs/3131992770'

@github-actions
Copy link

@iamanastasia
Copy link
Contributor

Hi @MatthewCYLau and @drewcook New fixes of the mobile menu color affected current nav bar color for the desktop
Screenshot 2022-09-27 at 14 36 46

Can we fix that before publishing?
So in light theme header background will be 2B2D3C (style is called header)
Screenshot 2022-09-27 at 14 37 33

In the dark theme it's 1B2030 (style is called header as well but dark mode)
Screenshot 2022-09-27 at 14 38 35

@MatthewCYLau
Copy link
Contributor Author

MatthewCYLau commented Sep 28, 2022

👋 @drewcook Thank you for picking-up the feedback improvement points off my branch 🙏 Would you like me to close this particular PR, so that we could focus on your PR 👉 #1178

With regards to the nav bar colour issue @iamanastasia has kindly spotted, @drewcook on your PR branch, would you be able to help revert my changes inside of AppHeader.tsx on line 97 to bgcolor. I mis-understood the requirements when I made the change.

Thank you all! 🙏

@drewcook
Copy link
Contributor

@MatthewCYLau Yes, I'll rollback those changes. We will close this PR in favor of #1178.

Thank you so much for your contribution on this! Amazing work!

@drewcook drewcook closed this Sep 28, 2022
@MatthewCYLau MatthewCYLau deleted the feat/578-set-mock-wallet-address branch September 28, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting mockWalletAddress via ui
7 participants