Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Feature) [Spending Limit] Settings tab#1151

Merged
fernandomg merged 2 commits intofeature/#413-SpendingLimit-in-appfrom
feature/#689-settings-tab
Jul 29, 2020
Merged

(Feature) [Spending Limit] Settings tab#1151
fernandomg merged 2 commits intofeature/#413-SpendingLimit-in-appfrom
feature/#689-settings-tab

Conversation

@fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Jul 23, 2020 •
edited
Loading

This PR closes #689, by adding Allowances Spending Limit menu in settings tabs.

I'm not so sure how this scales, the button going far to the right, too much white space. Hope @alongoni can give me a hint here.

image

I've created a branch feature/#413-SpendingLimit-in-app as a sort of epic branch. That branch will finally be merged to development once the feature is approved.

@fernandomg fernandomg added Major Needs to be fixed for immediate next public release. Feature 👑 New functionality labels Jul 23, 2020
@fernandomg fernandomg self-assigned this Jul 23, 2020
@github-actions
Copy link

github-actions bot commented Jul 23, 2020 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Jul 23, 2020

Travis automatic deployment:
https://pr1151--safereact.review.gnosisdev.com/app

@lukasschor
Copy link
Contributor

lukasschor commented Jul 24, 2020

Allowances should be called "spending limits". Do you need the designs to be updated to reflect this, or is it possible for you to just make the copy changes yourself manually?

@fernandomg fernandomg changed the base branch from feature/#413-allowances-in-app to feature/#413-TimeLimit-in-app July 24, 2020 12:26
@fernandomg
Copy link
Contributor Author

fernandomg commented Jul 24, 2020

Allowances should be called "spending limits". Do you need the designs to be updated to reflect this, or is it possible for you to just make the copy changes yourself manually?

oops! no need, working on it. Thanks

@fernandomg fernandomg changed the title (Feature) [Allowances] Settings tab (Feature) [Time Limit] Settings tab Jul 24, 2020
@fernandomg fernandomg changed the title (Feature) [Time Limit] Settings tab (Feature) [Spending Limit] Settings tab Jul 24, 2020
@fernandomg fernandomg changed the base branch from feature/#413-TimeLimit-in-app to feature/#413-SpendingLimit-in-app July 24, 2020 12:28
@ghost
Copy link

ghost commented Jul 24, 2020

Travis automatic deployment:
https://pr1151--safereact.review.gnosisdev.com/app

@francovenica
Copy link
Contributor

francovenica commented Jul 24, 2020 •
edited
Loading

@lukasschor Does it has to look good for phones? or we don't care about that yet?. Also with the amount of tabs we are adding they also will look off.
If that is not an issue then I'm ok with this PR

image.png

@lukasschor
Copy link
Contributor

lukasschor commented Jul 27, 2020

Sasha has changed the icon that should be used for the spending limit feature. Could you exchange the icon in the settings sidebar with this one (from Zeplin), please?

image

@lukasschor
Copy link
Contributor

lukasschor commented Jul 27, 2020

@lukasschor Does it has to look good for phones? or we don't care about that yet?. Also with the amount of tabs we are adding they also will look off.
If that is not an issue then I'm ok with this PR

image.png

It doesn't have to work well on mobile devices.

@fernandomg
Copy link
Contributor Author

fernandomg commented Jul 27, 2020 •
edited
Loading

PR with the fuel icon: 5afe/safe-react-components#46

mmv08
mmv08 approved these changes Jul 28, 2020
View reviewed changes
src/routes/safe/components/Settings/SpendingLimit/style.ts
},
modal: {
height: 'auto',
maxWidth: 'calc(100% - 30px)',
Copy link
Contributor

@mmv08 mmv08 Jul 28, 2020

Choose a reason for hiding this comment

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

what is 30px?

Copy link
Contributor Author

@fernandomg fernandomg Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

a measure? honestly don't know if it's a requirement, this is replicated code from Advanced tab, and it's the initial PR of an epic.

I'm planning to do clean up when everything is in place and I'm sure I don't need things like this class.

@fernandomg fernandomg merged commit 0277adb into feature/#413-SpendingLimit-in-app Jul 29, 2020
@fernandomg fernandomg deleted the feature/#689-settings-tab branch July 29, 2020 14:21
@fernandomg fernandomg mentioned this pull request Sep 16, 2020
Closed
@fernandomg fernandomg mentioned this pull request Nov 20, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alongoni alongoni Awaiting requested review from alongoni

@nicosampler nicosampler Awaiting requested review from nicosampler

@matextrem matextrem Awaiting requested review from matextrem

@dasanra dasanra Awaiting requested review from dasanra

2 more reviewers

@mmv08 mmv08 mmv08 approved these changes

@Agupane Agupane Agupane approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fernandomg fernandomg

Labels

Feature 👑 New functionality Major Needs to be fixed for immediate next public release.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fernandomg @lukasschor @francovenica @mmv08 @Agupane