Skip to content

fix(span-tree): Sibling span multi groups#33211

Merged
0Calories merged 13 commits into
masterfrom
fix/sibling-span-multi-groups
Apr 4, 2022
Merged

fix(span-tree): Sibling span multi groups#33211
0Calories merged 13 commits into
masterfrom
fix/sibling-span-multi-groups

Conversation

@0Calories

Copy link
Copy Markdown
Contributor

This PR fixes a bug with autogrouped sibling spans, where clicking to expand/collapse one group would do the same for other groups that share the same operation and description.

@0Calories 0Calories requested a review from a team April 1, 2022 17:13
toggleSpanTree: () => void;
trace: Readonly<ParsedTraceType>;
treeDepth: number;
groupOccurrence?: number;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I chose to name it groupOccurrence here as opposed to just occurrence, since I thought it would be confusing without clarification, as this prop is part of just a regular spanBar

@k-fish k-fish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Assuming you get it passing 👍 , like the use of groupOccurrence for clarity.

| ((props: {eventSlug: string; orgSlug: string}) => void)
| undefined;
toggleSiblingSpanGroup: ((span: SpanType) => void) | undefined;
toggleSiblingSpanGroup: ((span: SpanType, occurrence?: number) => void) | undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know you switched it, but I actually think it shouldn't be optional, conceptually, if you are picking a group, it must have occurred.

@0Calories 0Calories Apr 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah true, I agree it shouldn't be optional at least for spanSiblingGroupBar, but I think it should be kept optional for spanBar, since this component is associated with a group only when it is part of an expanded sibling group. So regular spanBars won't have an occurrence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nevermind you're right, I changed it so it's required here as well, since the function should always receive an occurrence number.

@0Calories 0Calories merged commit 2cfc196 into master Apr 4, 2022
@0Calories 0Calories deleted the fix/sibling-span-multi-groups branch April 4, 2022 13:39
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants