Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create ForageVaultWrapper #144
Create ForageVaultWrapper #144
Changes from all commits
6225fba
f8c96db
e92873b
db10aa0
fb84ec6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning
Code scanning / Android Lint
Mismatched Styleable/Custom View Name Warning
Check warning
Code scanning / Android Lint
Mismatched Styleable/Custom View Name Warning
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.
Nice strategy for separating the PIN validation logic from the PIN rendering logic!
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.
omg embarrassing - thank you for catching and fixing!
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.
No action required right now but the work we're doing here is surfacing that we probably have some tech debt around the relationship between
VaultWrapper
andForagePINEdiText
that we'll want to pay down at some point.That we don't want
BTVaultWrapper
orVGSVaultWrapper
to implementgetForageTextElement
, indicates that we should not be makinggetForageTextElement
an abstract method on theVaultWrapper
class.VaultWrapper
already declaresgetTextElement
andgetVGSEditText
as abstract methods even though they too really should not be living onVaultWrapper
since they are specific to BT / VGS wrappers. After poking around the code, it's clear thatForagePINEditText
is implemented in a way that expects all vaults expose agetTextElement
andgetVGSEditText
.ForagePINEditText
s dependency on these methods means the fix won't be trivial and so we should probably defer paying this debt down to a later ticket.One way that I can see to fix this delema is to create an abstraction over the underlying editable text field that BT, VGS, and Forage expose so that there is a unified way of calling things like:
getUnderlyingView()
clearText()
But anyhow, my next step is to make a Linear ticket to capture this and share the link here once I do. But, like a said before, I think what you're doing here is fine and won't break or block anything.
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.
Introducing the
data class ParsedStyles
definitely made it easier to read the render logic insideForageVaultWrapper
- good idea!Curious why we put
ParsedStyles
inside ofVaultWrapper
instead of keeping it local toForageVaultWrapper
? From what I can tell, thisParsedStyles
does not seem to be used byBTVaultWrapper
orVGSVaultWrapper
and wanted to better understand.At any rate, where
ParsedStyles
and it's related logic lives should not hold up merging in this me PR as it's internal code that we can revisit at any pointCheck warning
Code scanning / Android Lint
Mismatched Styleable/Custom View Name Warning