-
Notifications
You must be signed in to change notification settings - Fork 70
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
chore(ui): basic human feedback rendering in calls table #2991
Conversation
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallPage/cost/costUtils.ts
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallPage/cost/costUtils.ts
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/common.ts
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableColumns.tsx
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableColumns.tsx
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableColumns.tsx
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableColumns.tsx
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableQuery.ts
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableQuery.ts
Outdated
Show resolved
Hide resolved
weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/callsTableQuery.ts
Outdated
Show resolved
Hide resolved
const field = inputField.startsWith('summary.weave.feedback') | ||
? inputField.replace('summary.weave.feedback.', '') | ||
: inputField; | ||
const deBracketed = field.replace(/\[.*\]/g, ''); |
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.
the fact that this is looking for a bracket does not make sense to me given the comment above
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 think this is just a no-op right now
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.
ill update the comment, but I am using this fn in one place where we go from backend filter -> displayName
export const getFieldLabel = (field: string): string => {
if (field.startsWith('feedback.')) {
// Here the field is coming from convertFeedbackFieldToBackendFilter
// so the field should start with 'feedback.' if feedback
const parsed = parseFeedbackType(field);
if (parsed === null) {
return field;
}
return parsed.displayName;
}
return FIELD_LABELS[field] ?? field;
};
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.
hmmm, i would probably split those, but sure. It seems like a sufficiently different case
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.
hmmm, i would probably split those, but sure. It seems like a sufficiently different case
@@ -108,11 +109,12 @@ export const useCallsForQuery = ( | |||
{ | |||
skip: calls.loading, | |||
includeCosts: true, | |||
includeFeedback: true, |
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.
includeFeedback: true,
Description
MVP pr to combine feedback into the main calls data object.
https://wandb.atlassian.net/browse/WB-21666
Testing
How was this PR tested?