save textnode movement nad #126
Conversation
Signed-off-by: maissa SOUISSI <[email protected]>
src/components/network-area-diagram-viewer/network-area-diagram-viewer.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: maissa SOUISSI <[email protected]>
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| textNodeMoves[1].xOrig, | ||
| textNodeMoves[1].yOrig | ||
| ); | ||
| if (callMoveTextNodeCallback) { |
There was a problem hiding this comment.
move test over this.onMoveTextNodeCallback here
if (this.onMoveTextNodeCallback != null && callMoveTextNodeCallback) {
There was a problem hiding this comment.
This should create another bug I think. If you don't define a onMoveTextNodeCallback function then moving textNode should not work at all
There was a problem hiding this comment.
why? still able to move node even if onMoveTextNodeCallback (tested)
| textNodeMoves[1].yNew, | ||
| textNodeMoves[1].xOrig, | ||
| textNodeMoves[1].yOrig, | ||
| mousePosition |
There was a problem hiding this comment.
Why do you need mousePosition here ?
There was a problem hiding this comment.
mousePosition to save the newPosition of dragged text node
Signed-off-by: souissimai <[email protected]>
| @@ -208,6 +228,7 @@ export class NetworkAreaDiagramViewer { | |||
| } | |||
|
|
|||
| const dimensions: DIMENSIONS | null = this.getDimensionsFromSvg(); | |||
| console.log(' ======== dimensions ', dimensions); | |||
| //Update css rules to diplay labels | ||
| const firstChild: HTMLElement = <HTMLElement>this.svgDraw?.node.firstChild; | ||
| this.injectDisplayCssRules(firstChild); |
There was a problem hiding this comment.
What is the purpose of this ?
There was a problem hiding this comment.
the purpose is to reapply css rules to display labels
| let styleTag = htmlElementSvg.querySelector('style'); | ||
| if (!styleTag) { | ||
| htmlElementSvg.appendChild(document.createElement('style')); | ||
| console.debug('[injectDynamicCssRules] Style tag missing from SVG file. It has been created.'); |
| styleTags.forEach((styleTag) => { | ||
| styleTag.textContent = | ||
| styleTag?.textContent !== null | ||
| ? styleTag?.textContent.replace(`{display: none;}`, `{display: block;}`) |
There was a problem hiding this comment.
This line bothers me : there is an unnecessary "?", and if the css rule is slightly different, it will not work.
There was a problem hiding this comment.
if using a ternary operator impacts readability then switch to if..else code format I think
Signed-off-by: souissimai <[email protected]>
|
|
|
||
| // update the styles to display labels if needed | ||
| cssSelectors.forEach((selector) => { | ||
| const styleTags = [...document.querySelectorAll('style')].filter((styleTag) => |
There was a problem hiding this comment.
You could optimize this code a lot
You should call document.querySelectorAll('style') only once
Then filter styles nullish values .filter(Boolean) and not matching cssSelectors.
and finally loop over to set textContent.
This way you could avoid optional chaining operator elsewhere styleTag?.
By the way you have a problem because you have 2 local variables with the same name styleTag in injectDisplayCssRules and it's confusing...
Do you want to update the style on the given HTMLElement ? or on all Elements of the document ?
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
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: