Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for top-down scalefactor #153

Merged
merged 32 commits into from
Feb 28, 2024
Merged

support for top-down scalefactor #153

merged 32 commits into from
Feb 28, 2024

Conversation

Eddykasp
Copy link
Contributor

@Eddykasp Eddykasp commented Sep 8, 2023

corresponding server side PR kieler/KLighD#173 but doesn't rely on it to not break things

Comment on lines 176 to 181
let topdownScaleFactor = 1
if ((element.parent as any).properties == undefined || (element.parent as any).properties['org.eclipse.elk.topdown.scaleFactor'] == undefined) {
topdownScaleFactor = 1
} else {
topdownScaleFactor = (element.parent as any).properties['org.eclipse.elk.topdown.scaleFactor']
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scale factor already is 1, so just inverting the condition would make an else clause unnecessary. Or, why not write this as a one-liner with a fallback like this: const topdownScaleFactor = (element.parent as any).?properties['org.eclipse.elk.topdown.scaleFactor'] ?? 1

And: is this any cast necessary?

(multiple occurances of this in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion for simplifying the code, I'll have to check the any cast

@@ -369,6 +386,7 @@ export class DepthMap {
*/
computeDetailLevel(region: Region, viewport: Viewport, relativeThreshold: number, scaleThreshold: number): DetailLevel {
if (!this.isInBounds(region, viewport)) {
console.log(region.boundingRectangle.id + " out of bounds")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug log or message important for the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was for debugging and will be removed

@Eddykasp Eddykasp marked this pull request as ready for review January 22, 2024 09:21
change == to ===
please the prettier
really please the prettier
@NiklasRentzCAU NiklasRentzCAU added the enhancement New feature or request label Feb 28, 2024
@NiklasRentzCAU NiklasRentzCAU self-assigned this Feb 28, 2024
@NiklasRentzCAU NiklasRentzCAU merged commit caeff1b into main Feb 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants