Comments for ARankingView #235
Conversation
Don't know what it is good for, works without
src/views/AView.ts
Outdated
| @@ -109,7 +109,7 @@ export abstract class AView extends EventHandler implements IView { | |||
| return null; | |||
| } | |||
|
|
|||
| private buildParameterForm(params: HTMLElement, onParameterChange: (name: string, value: any, previousValue: any) => Promise<any>): Promise<IForm> { | |||
| private buildParameterForm(params: HTMLElement, onParameterChange?: (name: string, value: any, previousValue: any) => Promise<any>): Promise<IForm> { | |||
There was a problem hiding this comment.
The init function required to pass an event listener, but I didn't need to handle these changes so I made it an optional parameter.
There was a problem hiding this comment.
sorry, I overlooked this sentence. but what do you mean with "but I didn't need to handle these changes so I made it an optional parameter."? in which application didn't you have to handle these changes?
you could also pass a function like () => null as an event listener. would this work for you?
is there a good reason making this parameter optional instead of passing a function doing nothing?
There was a problem hiding this comment.
The RankingView already updates the content in the HTMLElement ("Showing 516 of 1013 Cell Lines; 11 selected"). I didn't have to update anything in our app based on this and therefore passed undefined.
That worked fine but I thought it will be clearer for future users if it is optional.
There was a problem hiding this comment.
ok, I think I see the problem. the callback passed to init is actually to track parameter changes in the provenance graph, not for updating the lineup stats. the stats are updated here: https://github.com/datavisyn/tdp_core/blob/keckelt/comments/src/lineup/ARankingView.ts#L573-L588
the onParameterChange callback is actually used as an onChange callback when you define define parameter input elements by overriding https://github.com/datavisyn/tdp_core/blob/keckelt/comments/src/views/AView.ts#L136-L143 .
the ViewWrapper of tdp_core usually already handles passing the onParameterChange callback https://github.com/datavisyn/tdp_core/blob/keckelt/comments/src/base/ViewWrapper.ts#L163 - so this really is for detail views and therefore I wouldn't make it optional
I probably still misunderstand something... then I'll need more context about your application
There was a problem hiding this comment.
Ok - I didn't use a ViewWrapper which might have made things unnecessary complicated: https://github.com/Caleydo/cohort/blob/87104e79a23585f8d919d5ee02aec3d76d9a682f/src/detail/DetailView.ts#L72-L98
|
I made the parameter non-optional again :) |
|
nice, thanks! 🙂 |
I added some comments while integrating an ARankingView into our application. Mainly for the constructor and its parameters.
The init function required to pass an event listener, but I didn't need to handle these changes so I made it an optional parameter.