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(popup): add ouf of funds and add funds screens #439

Merged
merged 11 commits into from
Jul 23, 2024
Merged

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Jul 22, 2024

Context

Changes proposed in this pull request

  • Adds Out of Funds (main) error screen and Add Funds screen
  • Add Funds pre-fills previous amount as default. In future, we can suggest custom amounts.
  • Add Funds screen supports back-arrow navigation to main screen #424
    • by using a route pattern (here, if includes /s/ in URL (s for screen or sub-page). It allows going back to parent - path before /s/.

@github-actions github-actions bot added area: popup Improvements or additions to extension popup area: i18n labels Jul 22, 2024
@raducristianpopa
Copy link
Member

raducristianpopa commented Jul 22, 2024

Extension builds preview

Name Link
Latest commit 803fe67
Latest job logs Run #10061974763
BadgeDownload
BadgeDownload

@sidvishnoi sidvishnoi changed the title Out of funds UI feat(popup): add ouf of funds and add funds screens Jul 22, 2024
@sidvishnoi sidvishnoi marked this pull request as ready for review July 22, 2024 13:24
Comment on lines +15 to +22
if (location.pathname.includes('/s/')) {
return (
<Link to={location.pathname.split('/s/')[0]}>
<ArrowBack className="h-6" />
</Link>
)
}

Copy link
Member Author

@sidvishnoi sidvishnoi Jul 22, 2024

Choose a reason for hiding this comment

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

This solves #424.

We can store location in localStorage, and use that as default route (initialEntries) in future, instead of managing screens from within components (at expense of using more pages) - to handle cases when we need to re-open previous route after popup-reopen (#366)

@sidvishnoi
Copy link
Member Author

Demo (to show what texts are shown where and the interactions)

Screencast.from.22-07-24.08.05.45.PM.IST.webm

Copy link

@tselit tselit left a comment

Choose a reason for hiding this comment

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

Approved, as agreed in the comments section

Comment on lines +53 to +56
{
path: ROUTES_PATH.OUT_OF_FUNDS_ADD_FUNDS,
lazy: () => import('./pages/OutOfFunds_AddFunds')
},
Copy link
Member

Choose a reason for hiding this comment

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

Good approach. Let's avoid having other folders in pages. For the future, defining sub-routes should happen like this.

src/popup/components/OutOfFunds.tsx Outdated Show resolved Hide resolved
@sidvishnoi sidvishnoi merged commit 679e0ed into main Jul 23, 2024
8 checks passed
@sidvishnoi sidvishnoi deleted the out_of_funds_UI branch July 23, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: i18n area: popup Improvements or additions to extension popup
Projects
None yet
5 participants