Add labels to split view sandcastles #12759
Conversation
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
mzschwartz5
left a comment
mzschwartz5
left a comment
There was a problem hiding this comment.
Looks good to me - only the one comment about potentially sharing CSS, otherwise gtg!
| cursor: ew-resize; | ||
| } | ||
|
|
||
| .split-label { |
There was a problem hiding this comment.
Do sandcastle's have any shared CSS file where we could put this rather than copying it to each?
There was a problem hiding this comment.
The shared styles are mostly in that bucket.css that's imported above. This is a broader question about how we manage styles for sandcastles in general. Will probably aim to tackle this as part of the sandcastle rebuild.
We already duplicate the styles for the #toolbar element like this in the sandcastles that need it so this is not unusual for current sandcastles.
One concern with putting them all into a shared stylesheet is that they become "hidden" from the user. It's not obvious they exist and not easy to discover how to use them without copying from an existing sandcastle. Plus trying to replicate a sandcastle without these styles will make it not display correctly. It's a balance that needs a bit more thought, more than this small PR 😆
There was a problem hiding this comment.
Related: There's one thing that always bugged me about the slider, far more "user-facing" than some CSS:
It is related to the CSS, and that may make it more difficult to fix. But having to call document.getElementById is ... not ideal in general, IMHO, and I wondered whether this "element" could somehow be sneaked into the Cesium Viewer, and just be hidden by default.
The point is: That duplicated code should (or could?) preferably be some
viewer.enableSplitSlider(); // ?
viewer.installDefaultSplitHandler(); // Sets up all the default action mappings...
Who wants to drag that slider with the right mouse button after all? 😆
There was a problem hiding this comment.
By including it in the viewer itself it would also lock people into the way we want it to look. We could add options to style it but it's still under our control. Maybe that's a good thing, maybe not. Part of a bigger discussion. Maybe someone wants the "split viewer" functionality but no draggable line and instead a slider under it, or just a static width.
Doing it the way we have it creates some duplicated code in our sandcastle examples but makes it more flexible for everyone.
There was a problem hiding this comment.
Offering more fine-grained control does not mean that the boilerplate code that are currently required couldn't be offered as a convenience function. Analogously: What if users want to zoom with the left mouse button instead of the right? Yeah, you'll have to rewire a few things in the ScreenSpaceCameraController or so.
People who "just want to split that" can call justSplitThat(). People who want to make the slider 4 pixels wide and yellow can do this, but .... not with a makeItFourPixelsWideAndYellow() function, FWIW 😁
|
Thanks @jjspace! I didn't do a full review, but I have some wording suggetions: "Bing Maps unlabeled + Washington Imagery + Labels only" -> "Gaussian Splats / 3D Tiles Mesh" -> "3D Tiles Gaussian splats / 3D Tiles mesh" "3D Tileset / OSM Buildings" -> "3D Tiles mesh / 3D Tiles OSM buildings" |
|
Thanks @mzschwartz5 and @ggetz. I just pushed some updated wording. I think this is good to go |
|
@mzschwartz5, if you reviewed the implementation, please feel free to merge once you're happy with any changes. |
Description
Add clarity by including labels to sandcastles that use the split view. I'd appreciate a second pass on the wording to make sure all the labels are correct (@ggetz?).
Issue number and link
Fixes #12758
Testing plan
npm startSplitDirection(or part of this)SplitDirectionsandcastle itself because it'd dynamic and the focus is on the splitter not the content.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change