Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

HARP-12581: Move TextElement construction into builders. #1988

Merged
merged 5 commits into from
Nov 24, 2020

Conversation

atomicsulfate
Copy link
Collaborator

Cleanup text element construction code to remove duplication. Preparation to set renderOrder for text elements.

@atomicsulfate atomicsulfate marked this pull request as draft November 23, 2020 15:39
@atomicsulfate atomicsulfate force-pushed the HARP-12581_Refactoring branch 2 times, most recently from 222e25b to c776a54 Compare November 23, 2020 18:10
@atomicsulfate atomicsulfate marked this pull request as ready for review November 23, 2020 18:20
FraukeF
FraukeF previously approved these changes Nov 24, 2020
Copy link
Contributor

@FraukeF FraukeF left a comment

Choose a reason for hiding this comment

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

just some very minors... all in all I think this change is great and clears up a lot...
I wish we would not have the name "Poi" all over the code but something like "Marker" or "PointMarker" instead, but there is not much we can do about it at this point

import { assert, assertExists, LoggerManager } from "@here/harp-utils";
import * as THREE from "three";
import { Vector3 } from "three";
Copy link
Contributor

Choose a reason for hiding this comment

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

THREE is already imported use THREE.Vector3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -44,6 +39,38 @@ interface ImageTextureDef {
pixelRatio?: number;
}

function getPoiImageTexture(poiGeometry: PoiGeometry, index: number = 0): string | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

just another minor, maybe you can drop the Poi in the name as its inside the PoiManager, getImageTexture might be enough... same for the funtions below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #1988 (6265864) into master (d77100d) will increase coverage by 0.31%.
The diff coverage is 72.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1988      +/-   ##
==========================================
+ Coverage   65.88%   66.20%   +0.31%     
==========================================
  Files         294      295       +1     
  Lines       26262    26246      -16     
  Branches     5952     5910      -42     
==========================================
+ Hits        17304    17376      +72     
+ Misses       8958     8870      -88     
Impacted Files Coverage Δ
@here/harp-datasource-protocol/lib/DecodedTile.ts 50.96% <ø> (+0.96%) ⬆️
@here/harp-mapview/lib/text/TextElement.ts 93.24% <ø> (+6.82%) ⬆️
@here/harp-mapview/lib/poi/PoiManager.ts 12.85% <4.44%> (+3.49%) ⬆️
@here/harp-mapview/lib/poi/PoiRenderer.ts 44.35% <28.57%> (-0.49%) ⬇️
...e/harp-mapview/lib/geometry/TileGeometryCreator.ts 70.53% <85.71%> (+2.56%) ⬆️
@here/harp-mapview/lib/poi/PoiBuilder.ts 98.07% <98.07%> (ø)
@here/harp-mapview/lib/text/TextElementBuilder.ts 100.00% <100.00%> (ø)
@here/harp-mapview/lib/CameraMovementDetector.ts 81.69% <0.00%> (-11.27%) ⬇️
@here/harp-geometry/lib/SubdivisionModifier.ts 78.02% <0.00%> (-6.60%) ⬇️
...asource/lib/adapters/geojson/GeoJsonDataAdapter.ts 11.42% <0.00%> (-1.34%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d77100d...6265864. Read the comment docs.

@atomicsulfate atomicsulfate merged commit aabe350 into master Nov 24, 2020
@atomicsulfate atomicsulfate deleted the HARP-12581_Refactoring branch November 24, 2020 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants