Skip to content
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

Feature/550 remove report creation step 2 #564

Merged
merged 15 commits into from
Sep 13, 2022

Conversation

Elaine-Krauss-TCG
Copy link
Contributor

#550

Removes Report Creation Step 2

Copy link
Contributor

@mjtravers mjtravers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can be merged to develop after the code change is made to flatten the nested subscriptions as noted by Todd

@Elaine-Krauss-TCG
Copy link
Contributor Author

The switchmaps no longer have an unused variable, so that should no longer be an issue, @toddlees

image

@toddlees
Copy link
Contributor

The switchmaps no longer have an unused variable, so that should no longer be an issue, @toddlees

image

👍 I see you hid it in the response array. if you want to drop the tabledata after the observable resolves you can map it to report like below, but it works that way too!

    createReport$.pipe(
      switchMap((report) =>  getTableData$.pipe(map(()=>report))),
      switchMap((report)=> zip(of(report), cashOnHand$)),
      takeUntil(this.destroy$)
    ).subscribe(([report, cashOnHand]) => {

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

84.2% 84.2% Coverage
0.0% 0.0% Duplication

@mjtravers mjtravers merged commit 82c38e6 into develop Sep 13, 2022
@mjtravers mjtravers deleted the feature/550-Remove-Report-Creation-Step-2 branch September 13, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants