-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement Users page #2
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Just left some comments for minor improvements, looking awesome!
return env.randomFirstName().map(UsersAction.setFirstName).eraseToEffect() | ||
|
||
case .onDisappear: | ||
return .cancel(id: RandomGeneratorTimerId.self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we keep the RandomGeneratorTimerId
as part of the RandomGenerator
domain. For cancellation we could implement a cancel
method in the RandomGenerator
and just invoke it from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nanocba Thanks! Fixed!
|
||
struct UserInfoState: Equatable { | ||
var user: User | ||
var disabled: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been exploring with the idea of moving this type of variables to the view since this is merely view configuration that doesn't change at runtime.
text: viewStore.binding(get: \.job, send: UserInfoAction.jobChanged) | ||
) | ||
} | ||
}.disabled(viewStore.disabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this view more composable and also being able to remove the disabled
from state, you could remove this view modifier from here and just let the callers decide whether they want this view disabled or not i.e. UserDetailView
will apply this view modifier while EditUserView
will not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nanocba Thanks! Fixed!
Users page preview gif