[nad-viewer] create style and footer for legends generated from metadata#436
[nad-viewer] create style and footer for legends generated from metadata#436CBiasuzzi wants to merge 2 commits into
Conversation
Signed-off-by: Christian Biasuzzi <christian.biasuzzi@soft.it>
|
| y: 0, | ||
| }; | ||
| const busNodes: BusNodeMetadata[] = [busNode(0, '400 kV', 'background:red; fill:red; stroke:black;')]; | ||
| const legendBox = nadViewer['createLegendBox'](textNode, busNodes, node); |
There was a problem hiding this comment.
createLegendBox is private. Do we need a public constructor, or is it usual in typescript to test private methods like that ?
There was a problem hiding this comment.
It seems it is an acceptable way to test a private method (note also that the very same approach is already used by PR 430 )
| node.legendFooter?.forEach((footer) => { | ||
| newDivElement.appendChild(this.createTextHeader(footer)); | ||
| }); |
There was a problem hiding this comment.
this is the same thing as for legendHeader a bit above, line 1866
Could we make an helper function for that maybe ? If it's only in two places, maybe not needed, but if it's in other places it would be good.
Also, I think you should put this part just after the legendHeader (and not after the loop on busNodes)
There was a problem hiding this comment.
Thanks for the feedback.
It’s only used in those two places, I think it’s probably not worth extracting a helper for just those two lines that are involved.
Why shouldn’t I put the footer after the loop on the nodes? A footer belongs at the bottom, don't you think?
(see also this example: https://github.com/powsybl/powsybl-diagram/blob/main/network-area-diagram/src/test/resources/custom_label_provider.svg )
2 participants
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
Fixes #435
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: