Conversation
Generated by 🚫 Danger |
|
|
||
| @objc public func showTimezoneSelector() { | ||
| let controller = TimeZoneSelectorViewController(selectedValue: timezoneValue) { [weak self] (newValue) in | ||
| self?.navigationController?.popViewController(animated: true) |
There was a problem hiding this comment.
The screen now dismisses itself.
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 28011 | |
| Version | PR #24612 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 4f77d3a | |
| Installation URL | 537f156b7e098 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 28011 | |
| Version | PR #24612 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 4f77d3a | |
| Installation URL | 6ucd5l2dml2k0 |
|
Update: I also changed how suggestions are displayed to make it clearer what timezones is suggested and also reuse the code: 
|
|
|
||
| init(selectedValue: String?, onSelection: @escaping (WPTimeZone) -> Void) { | ||
| self.onSelection = onSelection | ||
| self._viewModel = StateObject(wrappedValue: TimeZoneSelectorViewModel(selectedValue: selectedValue)) |
There was a problem hiding this comment.
Does the issue mentioned in #24072 (comment) apply here too?
There was a problem hiding this comment.
Not in practice, but let's just get rid of it. We only need previously selectedValue and it can be a plain property - updated.
| func loadTimezones() async { | ||
| guard !isLoading else { return } | ||
|
|
||
| isLoading = true |
There was a problem hiding this comment.
I personally like to add a defer { isLoading = false } right below this line. The relevant code is kept together, and most importantly, prevents the issue where the function body returns in the middle of execution before isLoading = false is executed.
There was a problem hiding this comment.
I didn't write this – this entire file is generated 😄
I verified it all, and as long as it worked and was good enough for readability, I kept it as it to minimize the amount of busy work moving things around. I'd suggest to focus more on the substance as there is going to be some maybe sub-optimal constructs, but as long as they are correct and readable, I'd keep it as is. Do you agree?
There was a problem hiding this comment.
I forgot to mention this particular comment was a nitpick.
To your question about code quality, I don't think we should use a lower standard just because the code is generated by AI. It's in the codebase, it'll run in production, the same way our own code does.
Again, feel free to leave the isLoading code as it is. My suggestion was just a personal preference.
There was a problem hiding this comment.
I agree, I wouldn't lower the standards, and I change a lot of the code manually, including some style changes – so it's not just straight up one-prompt. In the future, I think it's a matter of adjusting the prompts to get it to follow more of the coding standards. It's a bit tricky because there aren't really well established and are not in writing.
In the example, there is an argument to be made that not using defer is simpler as it's one less relatively non-trivial language feature to use. The generate code tends to be simpler and use simpler primitives, which I personally prefer as it makes it easy to review. If it's simple and readable, I keep it as is optimizing for development speed.
kean
force-pushed
the
task/rework-timezone-picker
branch
from
5f468de to
10fe744
Compare
|
4 participants
TimeZonePickerViewControllerusing SwiftUI and withoutTimeZoneStore(WordPressFlux)RankedStringSearchand passing additional fields in searchScreenshots
Left – before. Right – after.
Here's an example where search would previously return poor result as it was looking was the wrong values:
Note: the screenshots are a bit old. I increased the cell height to closer match the original.