feat: aggregated balance UI #27161
feat: aggregated balance UI #27161
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
sahar-fehri
changed the base branch from
develop
to
feat/aggregated-balance-feature
|
Builds ready [7bb8de9]
Page Load Metrics (1813 ± 83 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/components/component-library/button-icon/button-icon.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hey Sahar, this is looking great, I'm just blocking this because I think there is an unnecessary update to a design system component that we may not want to include until we get confirmation. Also there are some other comments that should help maintain consistency and clean code. Happy to pair also!
Builds ready [34b2a84]
Page Load Metrics (1641 ± 63 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [52a8669]
Page Load Metrics (1556 ± 58 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [fa835b1]
Page Load Metrics (1733 ± 78 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Hey Sahar, feel free to merge to unblock, but just a heads up—using static CSS for font-size and font-weight is a code smell for the design system. We shouldn’t need to use those at all. We might even want to create a stylelint rule to prevent their usage. It's more likely the design isn't using the correct Figma library for Typography. If you remove all static declarations for font-size and font-weight, it will be design system approved! cc @amandaye0h
Builds ready [a771b75]
Page Load Metrics (1745 ± 96 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
This is looking great! Thank you for your diligence and openness to design system suggestions. I noticed a few small consistency updates were included, such as the settings headings. Breaking these changes into a separate PR would help with the review process, allow design system engineers to focus on details, and reduce the size of this PR. Just a suggestion for the future!
| @@ -3,7 +3,7 @@ | |||
| .dropdown { | |||
| position: relative; | |||
| display: inline-block; | |||
| height: 36px; | |||
| height: 48px; | |||
There was a problem hiding this comment.
It may be worth doing a check of all the instances that this might effect to ensure there are no visual regressions
ui/components/component-library/button-icon/button-icon.types.ts
Outdated
Show resolved
Hide resolved
| @@ -1558,6 +1558,11 @@ export const getConnectedSitesList = createDeepEqualSelector( | |||
| }, | |||
| ); | |||
|
|
|||
| export function getShouldShowAggregatedBalancePopover(state) { | |||
| console.log('state.metamask', state.metamask); | |||
There was a problem hiding this comment.
Also fine with you handling this on your feature branch so we don't block here.
| @@ -74,6 +74,7 @@ export default class AppStateController extends EventEmitter { | |||
| // States used for displaying the changed network toast | |||
| switchedNetworkDetails: null, | |||
| switchedNetworkNeverShowMessage: false, | |||
| shouldShowAggregatedBalancePopover: true, // by default user should see popover; | |||
There was a problem hiding this comment.
Would be cool one day if we could scope all of these "onboarding" related state variables within a single object, like onboardingState: {} to make it easier to understand which vars are related to onboarding vs what's related to the root level metamask state (unrelated to onboarding)
There was a problem hiding this comment.
Gonna approve this as is. As we discussed, there's an issue where the popover doesn't show on Testnet (works as expected on Mainnet). May want to verify behavior with Product here.
Feel free to address this, along with my other console.log nit on your feature branch. If you end up making changes here just ping me on slack so I can re-approve.
|
I think fullscreen looks a little worse than before, being left aligned with all that space. That said, I don't have a perfect suggestion for what it should do instead. 
|
I also noticed this. This is per the designs spec though. I also felt like it was a bit jarring (maybe I'll get used to it) Maybe the monetization buttons can get flexed to the right of the window, rather than be vertical aligned... |
sahar-fehri
merged commit b3e3fa2
into
feat/aggregated-balance-feature
I think Amanda and Hester are aligned on this; this is not the final state but its an okay state for couple of weeks |
Description
Aggregated balance UI feature;
This PR fixed UI designs related to the aggregated balance feature;
It also fixes the popover behavior;
Users should be able to see the popover by default and should see it only once.
Related issues
Fixes:
Related:
https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=4098-126757&node-type=instance&focus-id=4186-130770&m=dev
https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=4496-20506&node-type=FRAME&m=dev
Manual testing steps
Screenshots/Recordings
Before
After
Screen.Recording.2024-09-17.at.14.02.40.mov
Pre-merge author checklist
Pre-merge reviewer checklist