-
Notifications
You must be signed in to change notification settings - Fork 9
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
Third party asset deposit rule - local #679
Third party asset deposit rule - local #679
Conversation
…ird-party-asset-deposit-rule
…ird-party-asset-deposit-rule
…hird-party-asset-deposit-rule
...s/Features/AccountPreferencesFeature/Children/ThirdPartyDeposits/ResourcesList+Reducer.swift
Outdated
Show resolved
Hide resolved
if !viewStore.resources.isEmpty { | ||
listView(viewStore) | ||
} | ||
Spacer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacer needed? should use minLength: 0
if so?
Sources/Features/AccountPreferencesFeature/Children/ThirdPartyDeposits/ResourcesList+View.swift
Outdated
Show resolved
Hide resolved
Group { | ||
if case let .allowDenyAssets(exceptionRule) = viewStore.mode { | ||
Picker( | ||
"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the implications of using ""
here? I think for accessibility sake we wanna set it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It describes the purpose, meant to add it, but forgot. Thx for reminder.
} | ||
.padding(.leading, .medium3) | ||
|
||
Spacer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
.padding(.large3) | ||
.background(.app.background) | ||
|
||
Spacer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing Spacer
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, to push the content of the VStack on top.
.keyboardType(.asciiCapable) | ||
.autocorrectionDisabled() | ||
|
||
WithControlRequirements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should WithControlReq...
be in a footer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this scenario no, the button is shown near the text field as it is the only UI element in the screen.
|
||
case .allowDenyAssets: | ||
state.destinations = .allowDenyAssets(.init( | ||
mode: .allowDenyAssets(.allow), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.allowDenyAsserts(.allow)
is a little hard to understand, does this mean we start in segment allow
, a label would help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will add a label
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Looks great overall, left some potentially non-helping comments :P
Brings in Third Party deposits:
Video -> https://drive.google.com/file/d/1530qDqWas1-PUNsfk82zOmbdu70dQHhx/view?usp=drive_link