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

Handle state change in TreatmentView.tsx #1923

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gaugup
Copy link
Contributor

@gaugup gaugup commented Jan 24, 2023

Description

Now that causal component can update if the global cohort changes, there was a bug in TreatmentView.tsx where new treatment policy for a new global cohort wouldn't update the Treatment chart. The fix is to check if the number of policy samples are different in the current props and the previous props and update the state if they are unequal.

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

Signed-off-by: Gaurav Gupta <gaugup@microsoft.com>
) {
if (
prevProps.data[0].local_policies.length !==
this.props.data[0].local_policies.length
Copy link
Contributor

@vinuthakaranth vinuthakaranth Jan 24, 2023

Choose a reason for hiding this comment

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

Could there be a case where length is same but the policies have been updated?
can we use _.isequal(prevprops, currentprops) in that case?
Why is it always comparing with 0th index in prevProps.data[0] and this.props.data[0]? Should it take selectedIndex from state?

public componentDidUpdate(prevProps: ITreatmentViewProps): void {
if (
prevProps.data &&
this.props.data &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check for the length of data being at least 1, too?

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