-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(new-trace): Sorting trace drawer card content by subject. #71122
Conversation
<TraceDrawerComponents.SectionCard | ||
title={t('Emitted Metrics')} | ||
items={items} | ||
enableSorting |
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.
enableSorting | |
sortAlphabetically |
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 going to wait for others to review as we dont own this code. Besides the prop name change, the changes look fine to me
.sort((a, b) => { | ||
return a.subject.localeCompare(b.subject); | ||
}) |
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.
This will alter the order for context on issues details page as well. I preserved that order because it was what was there before, but I don't see a reason why we can't change it. Alphabetically seems good to me 👍 -- more of an FYI
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.
Heyo @leeandher, it only changes the order of the items inside the card. The context ordering is one level above this, it won't change.
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.
@Abdkhan14 .sort mutates the original array, so this might in fact change the order. It would be safer to create a copy of contextItems before sorting
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.
Oh I see, either way that order was also intentional. It displays the known items for each context before any additional items the user added from their SDK.
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.
Noted @leeandher, lets give sorting alphabetically a go.
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.
@Abdkhan14 .sort mutates the original array, so this might in fact change the order. It would be safer to create a copy of contextItems before sorting
@JonasBa, he mentioned the ordering of cards. We are sorting card > items.
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.
Code looks good, but are there any instances that don't get receive sortAlphabetically = true
? If not, you can probably skip the prop and just apply the sort for all renders of SectionCard
Yeah we don't sort the general Info sections. |
No description provided.