[PM-16916] Custom fields need label to be saved #1430
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1430 +/- ##
=======================================
Coverage 89.68% 89.69%
=======================================
Files 776 777 +1
Lines 48896 48965 +69
=======================================
+ Hits 43851 43917 +66
- Misses 5045 5048 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// The current alert shown. | ||
| private weak var currentAlertController: UIAlertController? |
There was a problem hiding this comment.
🤔 Even though it's weak I'm not very fond of having this reference here in order to apply this logic.
Could you check if you could add a kind of inversion of control in the AlertTextField -> textChanged so it calls a function passed to the AlertTextField which ends up updating a reference of the alert action?
There was a problem hiding this comment.
@KatherineInCode @matt-livefront what do you think?
There was a problem hiding this comment.
I agree. I'm also kind of surprised that this works because I'm not sure what's keeping a reference to the Alert to keep it from being deallocated. If Fede's suggestion doesn't work out, we already have an alert controller subclass here, could we use that house some of this logic?
There was a problem hiding this comment.
I also agree that we should try to do inversion of control here if possible.
| @@ -28,10 +31,12 @@ public class AlertAction { | |||
| /// | |||
| public init( | |||
| title: String, | |||
| shouldEnableAction: (([AlertTextField]) -> Bool)? = nil, | |||
There was a problem hiding this comment.
🤔 Despite our desire to alphabetize parameters, I still somewhat prefer parameters that take closures to be piled at the end to potentially take advantage of trailing closure syntax.
| /// The current alert shown. | ||
| private weak var currentAlertController: UIAlertController? |
There was a problem hiding this comment.
I also agree that we should try to do inversion of control here if possible.
…ttps://github.com/bitwarden/ios into cmcg/pm-16916-custom-fields-need-label-to-be-saved
BitwardenShared/UI/Platform/Application/Utilities/Alert/Alert/Alert.swift
Outdated
Show resolved
Hide resolved
BitwardenShared/UI/Platform/Application/Utilities/Alert/Alert/Alert.swift
Show resolved
Hide resolved
…Alert.swift pm-16916 Fix PR comment Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
🎟️ Tracking
PM-16916
📔 Objective
Disable OK button if there's no input.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes