Release 0.14.0 #518
Release 0.14.0 #518
Conversation
This addresses the middle bullet from #401 (comment)
This table has been appearing empty for a while, but this was not documented yet in an issue. Two mistakes in the code contributed to this: 1. Handlers for model.whenever in AnnotationPanel were reading arguments from the wrong position with incorrectly annotated type. 2. ItemMetadataView actually expects a Node, not a FlatItem. I fixed the issue by rectifying both of the above mistakes.
The lack of self-updating prevented the item metadata table from populating in a deep-linked annotation panel.
This was preventing focus on annotation deep link.
59d5713 fixed annotation focus on deep link, but broke focus on clickthrough to a different source (e.g. from a related items panel). By adding back an unconditional delay, both scenarios should work.
Besides the fact that this code needed serious cleanup, this also fixes the toggling behavior of the panel.
This enables a single related item to be focused at any given time, despite the related items being distributed over multiple groups.
There was a problem hiding this comment.
Excellent work @jgonggrijp. I have left a few remarks/questions.
The changed code makes sense. I have confirmed the bugs that are reported as fixed no longer occur.
The addition of MappedCollection goes beyond my understanding of the frontend, and maybe of TypeScript in general. I have read through the code but quickly got lost. I understand how and why such a collection would be used though, which is more important.
| <p class="title is-5">{{#if label}}{{label}}{{else}}{{text}}{{/if}}</p> | ||
| <p class="subtitle is-6">({{classLabel}})</p> | ||
| {{#text}} | ||
| <p class="title is-5">{{.}}</p> |
There was a problem hiding this comment.
What does the {{.}} here signify? I presume it is rendered with some variable from the view?
There was a problem hiding this comment.
In Handlebars and Mustache, {{.}} stands for the current context as a whole. For example, these data:
{
"text": "abcdef"
}with this template:
will give this output:
abcdef
| @@ -44,39 +44,36 @@ export default class AnnotationView extends CompositeView<FlatItem> { | |||
| this.render().listenTo(model, 'change', this.render); | |||
| } | |||
|
|
|||
| processAnnotation(annotation: FlatItem): void { | |||
| processAnnotation(model: FlatItem, annotation: Node): void { | |||
There was a problem hiding this comment.
model parameter does not appear to be used
There was a problem hiding this comment.
This method is an event handler. The event includes the model as the first payload argument. So while the method does not actually use the model, we still have to include it in the parameter list.
| click: this.onSummaryBlockClicked, | ||
| hover: this.onSummaryBlockHovered, | ||
| }, this); | ||
| this.listenTo(this.collection, 'focus blur', this.trigger); |
There was a problem hiding this comment.
What is triggered here? I'm familiar with trigger('someEvent')
There was a problem hiding this comment.
This is basically a forwarding construction. When this.collection triggers focus or blur, the view re-emits this same event with the same payload.
| // prevent this behavior here because the mapping is noninjective. This | ||
| // is a bit of a stop-gap solution, but it should pose no problem in | ||
| // this case. | ||
| this.relatedItems.stopListening(this.relations, 'remove'); |
There was a problem hiding this comment.
Good idea providing comment to explain this. Has the potential to cause unexpected behaviour.
|
|
||
| function announce() { | ||
| if (this.toolbarModel.get('annotations')) { | ||
| announceAnno.call(this); |
Where did you first get lost? That might be a location for some additional documentation. |
As previously discussed with @BeritJanssen and @JeltevanBoheemen, I prepared a release branch in which I swept up a substantial amount of bugs, both documented and undocumented. I also made a few minor enhancements. This is now finally ready for review. This release branch is currently deployed on the acceptance server, so it is easy to preview.
Summarizing the changes:
/annotationsprefix in the source panel's self-reported URL when applicable.[object Object]in the source metadata ( Source metadata creator always display "object Object" #511).MappedCollection, which does what the name suggests and is analogous toFilteredCollection.FlatItemCollectionshared between the predicate groups. This was necessary to maintain the invariant that at most one item receives focus at any given time. Two layers of mapping and one layer of filtering ensure that each subview gets its own subset of flat items from this common collection.focus/blurevents from the collection instead of onclickevents from the subviews. This is a good reminder that state-dependent transitions in the view layer should generally follow state changes in the model layer, rather than reacting directly to other events in the view layer.While I'm sure this is a major usability improvement overall, the code churn is substantial, so I think a critical code review is advisable.