GT-2219 languages available offline #1940
Conversation
…into GT-2219-Languages-Available-Offline # Conflicts: # godtools.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Hey @rachaelblue this is looking good! This PR was nice and simple to review.
Although the interface and repository takes some additional time to setup, it really does help separate logic that would be bloating the ViewModel.
Just a couple change requests and also shared some thoughts in ToolLanguagesAvailableOfflineView. Let me know your thoughts on those UI comments I had left.
...res/AppLanguage/Data/DownloadedLanguagesRepository/Cache/RealmDownloadedLanguagesCache.swift
Show resolved
Hide resolved
...ols/App/Features/AppLanguage/Data-DomainInterface/GetDownloadedLanguagesListRepository.swift
Show resolved
Hide resolved
| ToolLanguageAvailableOfflineLanguageView() | ||
| ScrollView(.vertical, showsIndicators: true) { | ||
|
|
||
| VStack(alignment: .leading, spacing: 0) { |
There was a problem hiding this comment.
Whoops I should've had LazyVStack here in the dummy setup.
| ToolLanguageAvailableOfflineLanguageView() | ||
| let buttonHeight: CGFloat = 50 | ||
| let spaceBelowScrollView: CGFloat = 25 | ||
| let scrollViewMaxHeight: CGFloat = scrollViewGeometry.size.height - buttonHeight - spaceBelowScrollView |
There was a problem hiding this comment.
I'm not too crazy about this calculation here for the scrollViewMaxHeight. I would prefer the layout to simply adapt without having to do any calculations.
Another reason is if we modify this view to add additional UI below the scrollView then we have to update scrollViewMaxHeight which may not be known to do. More bug prone that way.
Was their an issue with original layout structure. I think the way it was before the ScrollView would still wrap the list?
There was a problem hiding this comment.
The problem before was that the scrollView took up the maximum space and always forced the blue button to the bottom of the screen, even if there were only a few rows in the scrollView:
Whereas Figma has the button directly beneath the last row:
I was trying to figure out a way to do this without calculations, but just doing .fixedSize(horizontal: false, vertical: true) without also setting the maxHeight ends up pushing the blue button off the screen if there's too many rows. I can play with this some more and see if there's a better solution
There was a problem hiding this comment.
Hey @rachaelblue I see what you're saying now about the button sticking to the bottom of the screen originally.
It would be nice to have the scrollview wrap it's content, but then you're right, it seems that would introduce the possibility of the button getting pushed off screen.
It would be nice if there were some solution where that height didn't have to be calculated by explicitly telling it which spacings and element heights needed removing. It does open the possibility of creating a UI bug in the future. If you find yourself spending too much time here then I'd say create a ticket for it and it can be re-investigated later.
There was a problem hiding this comment.
Tried a few different things here, but none quite had the desired result. It seems that SwiftUI doesn't let you make the ScrollView shrink to fit its content while also ensuring the view stays within the safeArea, unless you explicitly set a maxHeight to keep it within the safe area. Went ahead and made a ticket: https://jira.cru.org/browse/GT-2287
There was a problem hiding this comment.
Hey @rachaelblue thanks for checking into that some more. You're right we would need some way to tell it not to expand beyond the safe area. Thank you for making the ticket too, maybe something will be available in the future to allow for that behavior.
| let spaceBelowScrollView: CGFloat = 25 | ||
| let scrollViewMaxHeight: CGFloat = scrollViewGeometry.size.height - buttonHeight - spaceBelowScrollView | ||
|
|
||
| VStack(alignment: .leading, spacing: spaceBelowScrollView) { |
There was a problem hiding this comment.
I think VStack and HStack spacing you have to be careful sometimes in how it's used. To me it makes more sense in lists of repeating elements or where a ForEach is used.
In this particular case it still works since the VStack contains only a ScrollView and Button, however there is the potential of this breaking if we later update this UI to include some additional elements between the Button and ScrollView. We'd most likely want variable spacing between those elements.
I think here it would be more preferred to have this spacing applied either as a bottom padding to the ScrollView or top padding to the Button.
| if completedDownloadsOnly { | ||
| realmDownloadedLanguages = realmDownloadedLanguages | ||
| .where { $0.downloadComplete } | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is what you meant or not 😄 but it's a little better since it returns the same Result type. Is there a different way to do it that I'm missing?
Originally I was doing the condition inside the filter, like
var downloadedLanguages = realmDatabase.openRealm()
.objects(RealmDownloadedLanguage.self)
.filter { completedDownloadsOnly ? $0.downloadComplete : true }
.map { DownloadedLanguageDataModel(realmDownloadedLanguage: $0) }
But thought there's no reason to perform the filter when completedDownloadsOnly = false just because it looks cleaner. I guess .where would also work in that ^ scenario. Seems like the only difference between .filter and .where is the return type. And maybe it's more readable to use the word "where" in cases like this.
There was a problem hiding this comment.
Hey @rachaelblue oh wow my eyes and my brain were seeing something different this morning.
For some reason I thought your else statement was filtering on objects where downloadComplete was false. But, if that were the case you could still use .filter and check if downloadComplete was equal to completedDownloadsOnly without the need for the conditional.
Sorry for the confusion there! I was seeing something different and my brain was thinking something different.
No description provided.