-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove the InProgress and Failure value param. #54
base: master
Are you sure you want to change the base?
Conversation
src/app/examples/pos/pos.html
Outdated
@@ -24,8 +24,7 @@ | |||
<div | |||
*ngIf=" | |||
(service.meow$ | async | isSuccess) || | |||
((service.meow$ | async | isInProgress) && | |||
(service.meow$ | async | inProgressValue)) | |||
(service.meow$ | async | isInProgress) |
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'm just briefly checking from my phone, I have not run the example. My understanding is that we don't need to show this element when it's in progress, since we don't have a cat anymore
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.
You are right 👍
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.
Looks good to me. Nice to see Prettier styling! I have only one minor suggestion in the meow example. It seems to me that we should not show the section with a cat anymore when it's in progress.
(service.meow$ | async | successValue)?.file || | ||
(service.meow$ | async | inProgressValue)?.file | ||
" | ||
[src]="(service.meow$ | async | successValue)?.file" |
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.
And if you agree and we show this section only when it's success, we won't need this check for null ?
anymore
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.
The ?
is needed because the successValue
pipe has some logic for providing a defaultValue
that allows the transform to return undefined
(because defaultValue
is optional.
src/app/examples/pos/pos.html
Outdated
@@ -23,8 +23,7 @@ | |||
<!-- Success --> | |||
<div | |||
*ngIf=" | |||
(service.meow$ | async | isSuccess) || | |||
(service.meow$ | async | isInProgress) | |||
(service.meow$ | async | isSuccess) |
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.
That works correct, but probably we should improve the style and simplify the code to be just <div *ngIf="service.meow$ | async | isSuccess">
(on a single line and without parentheses)
See #53 for context
Install