Show dataset extent in right menu tab #3371
Conversation
|
@philippotto I have two ways of implementing the |
app/assets/javascripts/libs/utils.js
Outdated
| const minZ = Math.min(...allZCoordinates); | ||
| const maxX = Math.max(...allXCoordinates); | ||
| const maxY = Math.max(...allYCoordinates); | ||
| const maxZ = Math.max(...allZCoordinates); |
There was a problem hiding this comment.
Can you refactor this to avoid repetition? E.g., lines 134 to 142 can be shortened by mapping [0, 1, 2], and for lines 143 to 148 should also be a shorter way. E.g., const min = [0, 1, 2].map(idx => Math.min(...allCoordinates[idx]))?
| function getDatasetExtentInVoxel(dataset: APIDataset) { | ||
| const datasetLayers = dataset.dataSource.dataLayers; | ||
| const allBoundingBoxes = datasetLayers.map(layer => layer.boundingBox); | ||
| const unifiedBoudingBoxes = aggregateBoundingBox(allBoundingBoxes); |
There was a problem hiding this comment.
typo in unifiedBoudingBoxes
| const extent = getDatasetExtentInLength(this.props.dataset); | ||
| const datsetExtents = [ | ||
| `${extentInVoxel.width} x ${extentInVoxel.height} x ${extentInVoxel.depth}`, | ||
| formatExtentWithLength(extent), |
There was a problem hiding this comment.
Slight refactoring potential: How about formatExtentWithLength also accepts a second parameter which is a function which formats a value. So, you would call formatExtentWithLength(extent, formatNumberToLength). And in the line above, you could write formatExtentWithLength(extent, x => x) since the pure voxel count is not formatted.
That way, these two lines would unified a bit and the x logic only exists once.
There was a problem hiding this comment.
Independent of that: I wouldn't define these two extents in an array and access them individually. Instead, maybe just inline them into the <td />s each?
| <td>{datsetExtents[0]}</td> | ||
| </tr> | ||
| <tr> | ||
| <td /> |
There was a problem hiding this comment.
Do you think it is clear from a UI perspective that this row displays the extent in voxels? Maybe it should read that somewhere (either in the first cell of the row) or "5 x 10 x 15 Voxel" or (more mathematically correct) "5 Voxel x 10 Voxel x 15 Voxel" or "5 x 10 x 15 Voxel³". What do you think, @MichaelBuessemeyer @daniel-wer ?
Ah, now I see this comment. Yes, the second version is definitely better! I think there is still potential to make it even more concise, though :) (see my other comment) |
* origin/master: Add debugging methods to visualize wireframe of buckets (#3402) fixed bug that selects a just removed layout (#3379) Added button to revoke admin rights in frontend (#3378) Revert "Add REST API versioning support (#3385)" (#3404) Add REST API versioning support (#3385) limit number of tasks to be created in one api request (#3386) Use correct volume download route in TracingStoreRpcClient (#3403) clearer error message when uploading nml for inaccessible dataset (#3390) Refactored dropdown item events (#3383) Show dataset extent in right menu tab (#3371)
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated migration guide if applicable[ ] Updated documentation if applicable[ ] Needs datastore update after deployment