Disable sort controls when fetching data #507
Conversation
| } | ||
| } | ||
| }.disabled(isSorting) |
There was a problem hiding this comment.
It's a bit disappointing that "disabled" doesn't automatically have some kind of style modification that demonstrates that it is disabled.
There was a problem hiding this comment.
Yes - it was pretty frustrating to see that the disabled state looks exactly the same as enabled.
| Text(filters.sortFilter.name) | ||
| .font(.uiLabelBold) | ||
| .foregroundColor(.textButtonText) | ||
| if isSorting { |
There was a problem hiding this comment.
Is isSorting necessary? Could you not replicate the same functionality with
if [.loading, .loadingAdditional].contains(libraryRepository.state) {Then it might be the case that you only need the changes in this function, and not the state var, or the change with body, or indeed the changeSort() below?
If this is the case, it might be worth adding a new property on the LibraryRepository (or ContentRepository) to proxy this "network operation in progress" concept.
I might however be overlooking things, in which case I apologise (=
There was a problem hiding this comment.
Hi @sammyd, yes - you are correct. I didn't need to provide an isSorting property. I originally thought I needed it so I can force a UI update. I removed the property and added your code as well as some additional code for the disabled check. It works exactly as before. 👍
I didn't add a property to the repository because I'm thinking I don't think I'll need to change the repo state in such a way. I was thinking I would add it if I needed to do so in a different part of the codebase. Do you think that's a good idea or would you prefer I add it to the repository?
Thanks!
There was a problem hiding this comment.
I don't think that it's necessary to add a property anywhere unless you want to. The only thing I was kinda alluding to is the repetition of the "check the state" code that appears twice. I think it's fine as it is 👍
| @@ -27,6 +27,7 @@ | |||
| // THE SOFTWARE. | |||
|
|
|||
| import SwiftUI | |||
| import Combine | |||
There was a problem hiding this comment.
Is this import still necessary?
| } | ||
| } | ||
| }.disabled(libraryRepository.state == .loading || libraryRepository.state == .loadingAdditional) |
There was a problem hiding this comment.
Should probably be consistent with line 108. So either this, or switch 108 to match this || statement, whichever you feel is clearer.
| }.disabled(libraryRepository.state == .loading || libraryRepository.state == .loadingAdditional) | |
| }.disabled([.loading, .loadingAdditional].contains(libraryRepository.state)) |
There was a problem hiding this comment.
Sounds good - I've updated the PR with your suggestions. Thanks!
At the moment, when the user taps the sorting button (popularity, most recent), the app will make a request to the server. This request can be very slow which means the end-user can keep tapping the sort button. This results with the sort state being out of sync with the actual returned results.
To solve this issue, I've locked the buttons. This means that the user won't be able to change the sort order once they tapped the button. I've changed the font of the button to signify it is no longer tappable.