Annotation UILayer and Tooltips#1004
Conversation
BryonLewis
force-pushed
the
client/ui-layer-tooltip
branch
from
318090b to
51188da
Compare
|
Reviewing now. Should this also work for polygons when rectangles are turned off? |
Nope that's an oversight on my part. That's probably something I should add in. |
| .search([frame, frame]) | ||
| .map((str: string) => parseInt(str, 10)); | ||
|
|
||
| rectAnnotationLayer.setHoverAnnotations(visibleModesRef.value.includes('tooltip')); |
There was a problem hiding this comment.
Don't access visibleModesRef here, visibleModes is already dereferenced as a parameter to this function.
subdavis
left a comment
subdavis
left a comment
There was a problem hiding this comment.
More thorough review this time.
| import { | ||
| defineComponent, PropType, Ref, | ||
| } from '@vue/composition-api'; | ||
| import { StateStyles } from 'vue-media-annotator/use/useStyling'; |
There was a problem hiding this comment.
Use relative imports. Cannot use aliases here.
| @@ -0,0 +1,46 @@ | |||
| import { createApp } from '@vue/composition-api'; | |||
| import { MediaController } from 'vue-media-annotator/components/annotators/mediaControllerType'; | |||
There was a problem hiding this comment.
This should be a relative import. In the published lib, this alias doesn't exist.
| dataList: { | ||
| type: Object as PropType<Ref<[string, number, number][]>>, | ||
| default: () => [], | ||
| }, |
There was a problem hiding this comment.
This isn't a descriptive tuple. Could it be an object instead? { x, y, type } or something like that?
If you don't want to change it, can you document what it is here?
| default: () => [], | ||
| }, | ||
| selected: { | ||
| type: Object as PropType<Ref<number|null>>, |
There was a problem hiding this comment.
PropType<Ref<...>> is a bit of a hack. Props shouldn't be refs, they should be normal values.
I understand that because this component is being mounted as the root of the application, there's no parent to drive changes in its props, but it took me a little bit of code reading to figure that out.
Could the UILayer class have a description comment block to describe this design choice, how it works, and what the special requirements of its widget components are?
| const element = parent.appendChild(div); | ||
| let id = element.getAttribute('id'); | ||
| if (id === null) { | ||
| id = 'default_name'; | ||
| element.setAttribute('id', id); | ||
| } |
There was a problem hiding this comment.
I don't understand this block. Why are you checking to see if a DOM element you just created has an attribute you didn't set? Won't id always be null?
| element.setAttribute('id', id); | ||
| } | ||
| const elementId = id; | ||
| createApp(component, props).mount(`#${elementId}`); |
There was a problem hiding this comment.
Moreover, why do we need to give the new element an ID at all? Mount takes an element or a selector, so you can just pass it element directly.
subdavis
left a comment
subdavis
left a comment
There was a problem hiding this comment.
Pulled and tested again. Thanks for these revisions.
2 participants
resolves #1000
TextLayer.tsinside of AnnotationLayers folder. I believe it belongs there because that text is directly related to the annotations.UILayer.tsclass which currently allows for the generation of GeoJS DOMWidgets. The widgets are added with a key name and will allow for multiple DOMWidgets to be bound to a single UI Layer in case we add more items in the future. This where there we would include z-ordering and some other functions that related to DOMWidgets and the layer if they are needed.EditorMenu.vueand layers type definition forVisibleAnnotationTypesrectangleLayer.tswas modified to includemouseoverandmouseoffevents. I didn't want these on all the time doingpointSearchif it wasn't necessary, so I connected them to a toggle so the events could be added and removed as the user changes them in theVisbleModesRefinLayerManager.vue. if therectangleLayer.hoverOnis true it will emit signals when the user enters or leaves an annotation. These are computed into a list and fed into the ToolTipWidget for displaying. The position of the tool tip is relative to the enter or exit area. I didn't want to constantly move the item in the DOM.typeStylingcolor function,stateStyles,selectedTrackIdRefand a reactive list of the [typeName, confidence, trackID] that are to be displayed. It takes these data and creates a simple v-card to display it.Stuff I'm not happy with and open to suggestions:
LayerManager.vue?refsand.valueinside of the Vue component. Would reactive be better?