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

Get flag details on details page #1634

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Conversation

KD1712
Copy link
Collaborator

@KD1712 KD1712 commented Jun 7, 2024

Get details on navigation. Issue to load details on page reload or url hit

@KD1712 KD1712 marked this pull request as draft June 7, 2024 14:25
@Yagnik56 Yagnik56 requested review from danoswaltCL and Yagnik56 June 7, 2024 14:25
@Yagnik56 Yagnik56 linked an issue Jun 7, 2024 that may be closed by this pull request
@danoswaltCL
Copy link
Collaborator

So I'm guessing the issue you're seeing is this: what's happening is that the data you see when you nav from root page is from the pre-existing flag data, not actually from the fetch-by-id response. It's a trick! Your set-up is working up to that point. You can spy on the featureFlags.entities data-store value in the dev tools, when you navigate from root, the selector takes the id from url and finds it in there already because it exists from in the paginated call. When you refresh, that call is absent, so it will be empty, and it won't work that way because you're missing this:

Please add this in the reducer to pop the flag into the data store on success:

on(FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess, (state, { flag }) => {
    return adapter.addOne(flag, {
      ...state,
      isLoadingFeatureFlags: false,
    });
  }),
  1. If you put that in there, couple other things:
  • you'll want to make sure to wait until "data" is defined because this is an async call and it can be undefined at first
    <app-common-section-card-list *ngIf="featureFlag">. It would be good to show a load-spinner if else.

  • I found that I needed to help angular realize there was a change with this.changeDetectorRef.detectChanges(); after setting this.featureFlag

Now when the data store is updated on success, the page will render it when ready.

@KD1712
Copy link
Collaborator Author

KD1712 commented Jun 10, 2024

So I'm guessing the issue you're seeing is this: what's happening is that the data you see when you nav from root page is from the pre-existing flag data, not actually from the fetch-by-id response. It's a trick! Your set-up is working up to that point. You can spy on the featureFlags.entities data-store value in the dev tools, when you navigate from root, the selector takes the id from url and finds it in there already because it exists from in the paginated call. When you refresh, that call is absent, so it will be empty, and it won't work that way because you're missing this:

Please add this in the reducer to pop the flag into the data store on success:

on(FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess, (state, { flag }) => {
    return adapter.addOne(flag, {
      ...state,
      isLoadingFeatureFlags: false,
    });
  }),
  1. If you put that in there, couple other things:
  • you'll want to make sure to wait until "data" is defined because this is an async call and it can be undefined at first
    <app-common-section-card-list *ngIf="featureFlag">. It would be good to show a load-spinner if else.
  • I found that I needed to help angular realize there was a change with this.changeDetectorRef.detectChanges(); after setting this.featureFlag

Now when the data store is updated on success, the page will render it when ready.

Yes i get it now

@KD1712 KD1712 marked this pull request as ready for review June 10, 2024 07:41
>exposures-card</app-feature-flag-exposures-section-card
>
</app-common-section-card-list>
<div class="content-container">
Copy link
Collaborator

@danoswaltCL danoswaltCL Jun 10, 2024

Choose a reason for hiding this comment

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

if we sub to the feature flag directly with an async pipe in template, we can do this and eliminate half of the component code:

<ng-container *ngIf="(featureFlag$ | async) as featureFlag; else loading">
  <app-common-section-card-list class="common-section-card-list">
    <app-feature-flag-overview-details-section-card [data]="featureFlag" section-card></app-feature-flag-overview-details-section-card>
    <app-feature-flag-inclusions-section-card [data]="featureFlag" *ngIf="(activeTabIndex$ | async) === 0" section-card>inclusions-card</app-feature-flag-inclusions-section-card>
    <app-feature-flag-exclusions-section-card [data]="featureFlag" *ngIf="(activeTabIndex$ | async) === 0" section-card>exclusions-card</app-feature-flag-exclusions-section-card>
    <app-feature-flag-exposures-section-card [data]="featureFlag" *ngIf="(activeTabIndex$ | async) === 1" section-card>exposures-card</app-feature-flag-exposures-section-card>
  </app-common-section-card-list>
</ng-container>

<ng-template #loading>
  <mat-spinner></mat-spinner>
</ng-template>

Copy link
Collaborator

@danoswaltCL danoswaltCL Jun 10, 2024

Choose a reason for hiding this comment

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

then in component code, you just need this, no need to manage anything about this subscription by hand, the "filter" pipe isn't really doing anything very helpful, and change detector ref can be eliminated

featureFlag$ = this.featureFlagsService.selectedFeatureFlag$

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah i'll need this as an observable like suggested above in order to complete some work I'm currently doing, let me know if this suggestion works for you, I tried it in my local and it works fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes works for me too

@danoswaltCL danoswaltCL merged commit bcf5903 into dev Jun 11, 2024
8 checks passed
@danoswaltCL danoswaltCL deleted the feature/get-flag-details-by-id branch June 11, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate get flag details by id on nav to details page
2 participants