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

UX Multichain: Added Footer to the home screen #20751

Merged
merged 10 commits into from
Oct 11, 2023
Merged

UX Multichain: Added Footer to the home screen #20751

merged 10 commits into from
Oct 11, 2023

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Sep 6, 2023

This PR is to add the footer

Screenshots/Screencaps

Screenshot 2023-09-06 at 2 49 23 PM

Screen.Recording.2023-09-06.at.2.49.36.PM.mov

Manual Testing Steps

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@NidhiKJha NidhiKJha marked this pull request as ready for review September 6, 2023 09:20
@NidhiKJha NidhiKJha requested a review from a team as a code owner September 6, 2023 09:20
@NidhiKJha NidhiKJha added team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead needs-ux-ds-review labels Sep 6, 2023
@SaraCheikh
Copy link

SaraCheikh commented Sep 6, 2023

Hi @NidhiKJha this looks good!! a small detail on the global actions modal:

-The cells/buttons, are actually full width (modal doesn't have lateral padding), so on hover state we wouldn't see any padding.

Screenshot 2023-09-06 at 11 31 03

@NidhiKJha
Copy link
Member Author

Hi @NidhiKJha this looks good!! a small detail on the global actions modal:

-The cells/buttons, are actually full width (modal doesn't have lateral padding), so on hover state we wouldn't see any padding.

@SaraCheikh I have added the select action modal updates here #20755

@darkwing
Copy link
Contributor

darkwing commented Sep 6, 2023

Wow, this is a great start! A few things I noticed:

1. Onboarding

I'm seeing the bottom bar during the end of onboarding:
SCR-20230906-jhov

We should hide this until onboarding is completed.

2. When clicking "send", I don't see the bottom bar

I think we will want to keep the bottom bar during sending, but maybe that's a product decision cc: @kevinghim

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Good start!

@SaraCheikh
Copy link

I think we will want to keep the bottom bar during sending
@darkwing we only allow navigation between wallet and connections so display the bottom navbar only in the wallet screen and connections screen, in all other screens we hide.

vthomas13
vthomas13 previously approved these changes Sep 7, 2023
Copy link

@Ameralameri Ameralameri left a comment

Choose a reason for hiding this comment

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

Approved

darkwing
darkwing previously approved these changes Sep 11, 2023
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

This is awesome!

@NidhiKJha NidhiKJha added the DO-NOT-MERGE Pull requests that should not be merged label Sep 22, 2023
@NidhiKJha NidhiKJha changed the title UX Multichain: Added Footer to the home screen (WIP) UX Multichain: Added Footer to the home screen Sep 22, 2023
@darkwing
Copy link
Contributor

@NidhiKJha What's the status on this?

@NidhiKJha
Copy link
Member Author

@NidhiKJha What's the status on this?

@darkwing We have the test failures because of the notifications in the home page like the backup SRP reminder. So, we need to fix that issue so that it doesn't overlap with the footer placement. Design is in Progress for this. But I am blocked by this issue

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Code is looking good but I have a question for design regarding expanded view. What happens when we have a large list of assets? Is there some scrolling logic that keeps the footer in view? Does having the footer near the bottom of the screen make it harder to find? cc @kylefiedler

@github-actions
Copy link
Contributor

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.

@darkwing darkwing changed the title (WIP) UX Multichain: Added Footer to the home screen UX Multichain: Added Footer to the home screen Oct 10, 2023
@darkwing darkwing removed the DO-NOT-MERGE Pull requests that should not be merged label Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (a6f058f) 68.61% compared to head (d1d5e41) 68.64%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20751      +/-   ##
===========================================
+ Coverage    68.61%   68.64%   +0.03%     
===========================================
  Files         1017     1017              
  Lines        40814    40833      +19     
  Branches     10900    10907       +7     
===========================================
+ Hits         28002    28028      +26     
+ Misses       12812    12805       -7     
Files Coverage Δ
...-notifications/multiple-notifications.component.js 69.23% <ø> (ø)
ui/pages/routes/routes.component.js 44.10% <44.44%> (+0.02%) ⬆️
ui/components/multichain/app-footer/app-footer.js 62.16% <40.91%> (+58.46%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [d1d5e41]
Page Load Metrics (1092 ± 342 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint80154105199
domContentLoaded701551032010
load8217191092713342
domInteractive701551032010
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 943 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@NidhiKJha NidhiKJha merged commit 5562f67 into develop Oct 11, 2023
9 of 10 checks passed
@NidhiKJha NidhiKJha deleted the footer-align branch October 11, 2023 16:14
@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2023
@metamaskbot metamaskbot added the release-11.4.0 Issue or pull request that will be included in release 11.4.0 label Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.4.0 Issue or pull request that will be included in release 11.4.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants