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/Settings)!: split settings into tabbed interface #657

Merged
merged 19 commits into from
Oct 16, 2024

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Oct 11, 2024

Context

Changes proposed in this pull request

@github-actions github-actions bot added the area: popup Improvements or additions to extension popup label Oct 11, 2024
Copy link
Contributor

github-actions bot commented Oct 11, 2024

Extension builds preview

Name Link
Latest commit faef3ad
Latest job logs Run #11365623457
BadgeDownload
BadgeDownload

@sidvishnoi sidvishnoi changed the title feat(popup/Settings): split settings into tabbed interface feat!(popup/Settings): split settings into tabbed interface Oct 11, 2024
@sidvishnoi sidvishnoi changed the title feat!(popup/Settings): split settings into tabbed interface feat(popup/Settings)!: split settings into tabbed interface Oct 11, 2024
Base automatically changed from refactor/extract-ConnectWallet-page to main October 14, 2024 10:53
@sidvishnoi sidvishnoi marked this pull request as ready for review October 14, 2024 11:10
@sidvishnoi
Copy link
Member Author

Screenshots

Wallet screen
Wallet screen, advanced expanded
Budget screen
Rate of pay screen
Rate of pay screen, WM disabled

@RabebOthmani
Copy link
Collaborator

@sidvishnoi few comments:

  • the read-only fields (wallet, key, remaining balance) look like they are editable. Maybe a darker grey as background would help?

  • For the rate of pay, I thought we were getting rid of the slider?

  • Rate of pay screen, there's a design on Figma ( Let's move the rate on top of the page, continuous payment toggle underneath it, and when it is disabled we show text 'Ongoing payments are now disabled. You can still
    make one time payments. '

  • What happens when the user changes the rate of pay or budget amount?

@sidvishnoi
Copy link
Member Author

For the rate of pay, I thought we were getting rid of the slider?

Eventually yes, but not in this PR. It'll get too big, and that input isn't as simple if done right.

What happens when the user changes the rate of pay or budget amount?

Rate of pay gets handled immediately, like before.
Budget can't be changed at the moment. It's a big change. Those fields are disabled/readonly.

@RabebOthmani
Copy link
Collaborator

Ok for rate of pay slider.
As for the budget, we can't just say something is not going to be done because it's big change. The requirement was never for it to be read-only.
In the last call, we agreed that they can change the budget but it will completely override the previous grant and starts immediately. @raducristianpopa keep me honest here

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Oct 15, 2024

We'd need UI to support edits first then. We need submit buttons somewhere, and a messaging that they'll need to connect wallet again (it'll open new tab to connect that is, so we can't just to do it in input).

@RabebOthmani
Copy link
Collaborator

Why do they need to connect to the wallet again?

@RabebOthmani
Copy link
Collaborator

Does that mean that switching between monthly or not will cause the same issues

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Oct 15, 2024

When they change budget (any part of it - amount or recurring), we need to create a new interactive grant (i.e. ask user for permission), which happens in a new tab. So, it's the same connect wallet process again, but only without the need to add key again.
(This is not an extension thing, it's how Open Payments is designed)

Does that mean that switching between monthly or not will cause the same issues

Yes. Same issues. Changing any part of budget/grant.

@RabebOthmani
Copy link
Collaborator

You mean the part on the browser to accept the grant? or the connect screen on the extension?
That also mean that the monthly toggle is the wrong control. A toggle is to make changes not just for visual effects

@sidvishnoi
Copy link
Member Author

You mean the part on the browser to accept the grant?

Yes.

That also mean that the monthly toggle is the wrong control. A toggle is to make changes not just for visual effects

Yes, wrong control if it's not meant to be editable. Point #3 in Slack

@RabebOthmani
Copy link
Collaborator

Ok. Settings should be adjustable. A user should have the ability to change the budget allocated to use with the extension and whether or not they want to renew it monthly.
They will have to accept the grant online. That is absolutely fine.
We will add a submit changes button that gets enabled if the amount or renew values change. Similar to connect, when they click , they get redirected to the browser.

Please do let me know if anything is not clear or if there are any technical limitations before deciding on requirements on your own.
budget

@sidvishnoi
Copy link
Member Author

Will do budget-edit support as a follow-up PR. Keeping this PR limited to tab-UI thing only.

@sidvishnoi sidvishnoi mentioned this pull request Oct 16, 2024
@sidvishnoi sidvishnoi merged commit cf97069 into main Oct 16, 2024
8 checks passed
@sidvishnoi sidvishnoi deleted the settings-tabs branch October 16, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: popup Improvements or additions to extension popup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The payment rate should be a global setting
3 participants