-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Introduce a UiState class for the architecture pages #11437
Comments
I like the idea, but I would argue against some third-party package like |
FYI, there is still documentation for the Flutter Architecture guidelines in the works. A guide with practical implementation details should land soon: #11414 and there will also be a cookbook recipe on the Command pattern. IMO the presented UiState has some similarities with the Command pattern that the compass_app example uses, as it also contains the When implementing the compass_app example, we evaluated refactoring ViewModels to extract the UI state into a separated object, but we couldn't see the advantage against just having those inside the ViewModel/ChangeNotifier. If anyone is interested in it, this was the discussion on the PR: flutter/samples#2398 I'd say it is up to the developer how they decide to organize the data in the ViewModel, if directly in the class or as a separated UI object. This could be added as a side note on the MVVM implementation pages. |
In fact, this is going to be mentioned in the upcoming docs: So I think that covers this request. You can see a preview here: https://flutter-docs-prod--pr11414-ew-app-architecture-case-st-5j24ywro.web.app/app-architecture/case-study/ui-layer |
I know your message was about the UiState, but I think it's probably worth creating a separated ticket to discuss possible alternatives to freezed in the compass_app example and the Flutter Architecture guidelines. |
This looks nice and I think it goes into the same direction. I am still not sure if freezed should be used here by just saying that it ensures immutability. This sounds like, there is no other way in Flutter directly to do that. Is there a way to support you? |
I will open a ticket, thank you 😄 |
I feel pretty strongly that we shouldn't add more complexity to the base architecture guide. (But I'm always open to being wrong.) But, as Miguel said, we did discuss this at one point, and I can imagine writing a Design Pattern recipe about this, as those are kind of like "extensions" for the base documentation. |
@miquelbeltran thanks for pointing out to the works in progress, definitely the mention covers this request, and I still have hope somewhere you could add a code snippet (side note: freezed is one between so many different ways).
by my experience in all these years, having a class where all the fields which contribute to the UI state are well specified helps to limit bugs, while defining them inside a ChangeNotifier can mix them with other fields which don't |
Page URL
https://docs.flutter.dev/app-architecture/guide
Page source
https://github.com/flutter/website/blob/main/src/content/app-architecture/guide.md
Describe the problem
Similarly to Android Architecture Components, the UI layer should introduce a well defined UiState class.
The use of a UiState class has many benefits, as it allows a clear declaration of what is part of the UI state, it helps reasoning about it and it helps handling it, without having the code scattered into another class.
Here one example of a generic UiState class I use (with freezed):
The UI state class doesn't necessarily need to include the loading and error, but keeping them included helps to represent better the formula:
because anything that change the UI has to originate from a change in the state, including the loading state where we usually want to show a loading indicator and the error state where we usually want to show a snackbar.
Having them included in the UiState class helps to reason in terms of a (in)finite state machine:
All the material taken from here, where you can find also a code repository, a video and the slides.
Expected fix
Adding a section titled "Defining the UI state" or similar.
Additional context
No response
I would like to fix this problem.
The text was updated successfully, but these errors were encountered: