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

feat(annotations): add ability to edit human span annotations #4111

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Parker-Stafford
Copy link
Contributor

resolves #4076

  • adds the ability to edit human span annotations
Screen.Recording.2024-08-01.at.4.57.01.PM.mov

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Aug 2, 2024
@@ -178,11 +178,6 @@ export function SpanAnnotationForm(props: SpanAnnotationFormProps) {
type="submit"
size="compact"
isDisabled={!isValid || !isDirty || isSubmitting}
onClick={() => {
// TODO: This is a bit of a hack as the form is not working in a dialog for some reason
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the form seems to be submitting fine now, and this is causing double submission causing uniquness constraint errors in the db. Removing this fixes

Before

image (10)

After

image

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yeah this form submission in a dialog had me scratching my head. I can pull down tomorrow to investigate. I needed this on my machine but maybe you are right to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was double submitting with, the safest thing if we're not sure if it double submits is to do the opposite of what i did and only do the onClick

@@ -72,7 +72,7 @@ async def patch_span_annotations(
(
models.SpanAnnotation.annotator_kind,
annotation.annotator_kind.value
if annotation.annotator_kind is not None
if annotation.annotator_kind is not None and annotation.annotator_kind is not UNSET
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when annotator kind was not passed in (cannot be updated), we were getting an issue reading value off of UNSET type

Copy link
Contributor

Choose a reason for hiding this comment

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

@anticorrelator can you fix up this part?

@@ -226,7 +238,7 @@ function SpanAnnotationsList(props: {
`}
>
{annotations.map((annotation, idx) => (
<li key={idx}>
<li key={`${idx}_${annotation.name}`}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this key was not unique enough and was causing issues on delete

Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

Hey @Parker-Stafford sorry for the churn. Can we have these annotations be in edit mode from the get go? We need to eliminate the number of clicks as much as possible for these.

Comment on lines 35 to 51
if (onEdit) {
items.unshift(
<Item key={AnnotationAction.EDIT}>
<Flex
direction="row"
gap="size-75"
justifyContent="start"
alignItems="center"
>
<Icon svg={<Icons.Edit2Outline />} />
<Text>Edit</Text>
</Flex>
</Item>
);
}
return items;
}, [onEdit]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make it so that annotations are always in edit mode for human? They already clicked on "annotate"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes makes sense will fix that up!

@@ -72,7 +72,7 @@ async def patch_span_annotations(
(
models.SpanAnnotation.annotator_kind,
annotation.annotator_kind.value
if annotation.annotator_kind is not None
if annotation.annotator_kind is not None and annotation.annotator_kind is not UNSET
Copy link
Contributor

Choose a reason for hiding this comment

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

@anticorrelator can you fix up this part?

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 2, 2024
query {
node(id: $spanId) {
... on Span {
...EditSpanAnnotationsDialog_spanAnnotations
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do the span details fragment here too? Or not necessary

fix: update key for list of span annotations for deleting and adding

fix double submission on create

remove unused

switch to view mode when mutation is completed

make human annotations always editable

disable save after submission

switch to form

remove use effect

fix ruff

clear feature flag

simplify package json
@mikeldking mikeldking force-pushed the parker/4076-editable-human-annotations branch from bc6bfd4 to 18ca63a Compare August 2, 2024 17:14
@Parker-Stafford Parker-Stafford merged commit 67cb9a2 into main Aug 2, 2024
12 checks passed
@Parker-Stafford Parker-Stafford deleted the parker/4076-editable-human-annotations branch August 2, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[annotations][UI] Edit human annotations and save
2 participants