Sandcastle - Nested gallery structure #12631
Conversation
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
|
@ggetz I tried to split the commits a little between the new gallery files and actual code changes if that makes it easier to review |
|
@ggetz I know there's still some scattered commented code and console logs. I'll do a cleanup pass on those before this gets merged but wanted to just validate the approach first. |
| </script> | ||
| <!-- <script type="module" src="./Sandcastle.js"></script> --> | ||
| <script src="__CESIUM_BASE_URL__/Cesium.js" vite-ignore></script> | ||
| <script type="module"> |
There was a problem hiding this comment.
Do we need to set window.CESIUM_BASE_URL here as well?
There was a problem hiding this comment.
It still needs to be set somewhere and in a file that's processed by Vite to get the correct path based on the env it's building for. I think this is still the best way to do that. Let me know if you have a better idea.
| if (host.includes("localhost")) { | ||
| document.title = `${host.replace("localhost:", "")} ${document.title}`; | ||
| } else if (host.includes("ci")) { | ||
| document.title = `CI ${document.title}`; |
There was a problem hiding this comment.
What's the goal here? "CI" is an implementation detail for many who just want to view a demo.
There was a problem hiding this comment.
This was a bit of a quick patch mainly to help me while developing so I can tell which "environment" each tab I have open is in. This will probably get cleaned up more and include the actual title of a sandcastle for gallery items. I don't think "CI" is that bad here since it's very likely to only be running and deployed in our own CI. This wouldn't impact an actual "prod" version
There was a problem hiding this comment.
I think even in cases where something is deployed to branch, there's still cases where the Sandcastle is used for demo purposed. For instance, the recent Gaussian Splats work is being shown in example Sandcastles from that branch.
|
Reminder for myself, I realized the gallery is not working in CI. I think this is just a matter of not running the build gallery script in CI and boy updating the gallery url. I think this should be a quick fix but I'll take a look tomorrow |
|
@ggetz I've updated this PR to address most of your comments and responded to others. I think there's still a bit of cleanup I have yet to do but hopefully this is closer. The gallery has been solidified a bit and the route is now changed depending on the environment so the CI build now has a working gallery again. It also no longer gets copied and duplicated into the local build, instead it points to the |
| // TODO: there's probably a race condition here where the list might not be loaded yet | ||
| // I need to switch this to the more React declarative style |
There was a problem hiding this comment.
This part is already addressed in #12639
|
The
causing it to not even enter EDIT: I think that might work, if that's OK on other OSes. |
|
Good catch, sorry about that @javagl. That little bit of code is something I've used in another project based on this answer for how to detect if an es module is "main" but I've only ever used it on linux. Your suggested solution seems to still work fine so I pushed that up as a fix. Longer term if we integrate the build function with builds correctly we may not even need to keep that "is main" detection at all, but it's been helpful while building that script so I left it for now 🤷♀️ |
|
Beyond the comment above, I'm not in the position to do a profound technical review (regarding vite, tsx, or CI details). Details that may already be on the radar:
Some comments that may not belong here, but of which I don't know where they belong (maybe #12639 ?)
Some comments that may belong here: I have a bunch of Sandcastles from Forum support. They are just a bunch of
Yeah, the thumbnail layout is not perfect (and there is no horizontal scrollbar), but the technical structure seems to work nicely. It's not yet clear how exactly the old sandcastles will be "migrated" into the new structure. But given that the target structure is relatively simple (with clearly separated files, and each directory being "one sandcastle"), this shouldn't be too hard. |
|
@jjspace I'm taking another pass now, thank you for the updates! It'd be super helpful for me if you go back to your original comment and update description and your TODO list. It helps me understand the status. Are you still working through items in this PR, or is everything ready for final review before merge? |
|
I started working on the conversion script (pretty quick and working well) but I realized we forgot to think about the development only sandcastles How should we handle the development sandcastles? Still a separate directory? should it be nested? or should it just be a new label? or maybe a property of the yaml? I think there's 2 options I like
Adjacent question, does it make sense to support loading multiple galleries? probably not tooo difficult but not sure if it's worth This is most important if we go with option 2 above so we could load just the main gallery or the main gallery and the development gallery. @ggetz thoughts? |
| }; | ||
|
|
||
| // Convert the viewModel members into knockout observables. | ||
| Cesium.knockout.track(viewModel); |
There was a problem hiding this comment.
Totally out of scope of this PR, but we should really make a pass at getting rid of knockout from our examples. It leads to users depending on it, and ideally we want to move away from depending on it. Could you please write up an issue to take a content pass to get rid of the knockout bindings from Sandcastle examples?
There was a problem hiding this comment.
I think most of the places it's used in sandcastles right now are to set up sliders and more complex toolbar inputs. I was thinking this could be handled by expanding the Sandcastle "API" with more features. It's been in the back of my mind but I'll make sure it's written up somewhere
There was a problem hiding this comment.
Sounds good. I think it'll be good to write up an issue just in case we don't cover all the cases. If we end up doing so, then 🎉, we can close that issue.
Making it a property of the Ultimately though, either approach should be fine as long as there is one standard process and it is documented. |
Worth discussing, but could we park the implementation this for now and address it in a later PR? |
| }; | ||
|
|
||
| // Convert the viewModel members into knockout observables. | ||
| Cesium.knockout.track(viewModel); |
There was a problem hiding this comment.
Sounds good. I think it'll be good to write up an issue just in case we don't cover all the cases. If we end up doing so, then 🎉, we can close that issue.
| { | ||
| "include": ["./Sandcastle.ts"], | ||
| "compilerOptions": { | ||
| "tsBuildInfoFile": "../node_modules/.tmp/tsconfig.lib.tsbuildinfo", |
There was a problem hiding this comment.
So why do we need to set tsBuildInfoFile? And why is it using a path in node_modules?
There was a problem hiding this comment.
Without setting it it defaults to output this file in the same directory. I set it to inside the node modules just matching the pattern in the main tsconfig that was generated from the vite setup. the node_modules is already gitignored so it seems like a reasonable place to put some of these extra build related temp files.
There was a problem hiding this comment.
It's generally discouraged to write or modify any content to the node_modules folder, as it is inherently transitory. Please use another location and add it to the .gitignore as necessary.
What's the issue that arises when that file is written to the current directory? By default, this is written to next to the "out" location, so it may make sense to adjust the output location instead.
There was a problem hiding this comment.
It's generally discouraged to write or modify any content to the node_modules folder, as it is inherently transitory.
Generally I would agree with you. However this is following the example set by the default tsconfig set up with the vite template. As I understand it the tsbuildinfo file is also pretty temporary and only there to help speed up subsequent local builds.
What's the issue that arises when that file is written to the current directory?
Just extra noise in the file tree and needing to add it specifically to the gitignore.
Edit:
Looks like their motivation was the same as mine. It's just a nice easy place to put these temp files that's already under the gitignore umbrella with the rest of node_modules vitejs/vite#18435
If you still think they should be moved elsewhere just lmk where. I don't want to bikeshed on this
There was a problem hiding this comment.
But this is not built using vite, correct? It's built with tsc.
What vite does internally isn't so much my concern– But making sure our project follows good practices and is maintainable is. I'm concerned this approach is obscuring details by doing things in a non-conventional way, making things more difficult to understand and maintain for others in the future.
Sandcastle.js and Sandcastle.d.ts are already ignored correct? Perhaps the cleanest solution would be to set the output to a subdirectory, such as templates/dist and ignore the entire directory in the .gitignore.
There was a problem hiding this comment.
But this is not built using vite, correct? It's built with tsc.
It is built with vite when running the local dev server to allow for quicker updates when actively developing sandcastle directly.
What vite does internally isn't so much my concern– But making sure our project follows good practices and is maintainable is. I'm concerned this approach is obscuring details by doing things in a non-conventional way, making things more difficult to understand and maintain for others in the future.
The point I was trying to make is that the method I was using (tmp dir in node_modules) is what the vite scaffold project does by default. I extrapolated this to mean that that is the conventional or expected way to handle these files and is a "good practice". At least the expected way within vite projects.
Regardless, I've moved them all next to their configs by just removing the config path. It's easy to change later if we want
There was a problem hiding this comment.
Thanks for updating. We can circle back to this discussion and to vite practices as needed, but I think what's there now is just easier to understand at the end of the day.
|
I'm still attempting to test this out, by npm appears to have other plans... 
|
|
@jjspace Ok to handle this in a follow up PR, just let me know the plan. I noticed two things about "New" sandcastles:
import * as Cesium from "cesium";
const viewer = new Cesium.Viewer("cesiumContainer");
|
|
Otherwise, I tested this out by running the following commands and things appear to be running smoothly. |
It's not required but I'm fine encouraging it to follow closer to how people would use in their own applications.
They should work for both scenarios. Can you make sure the declaration file is generated for Sandcastle using |
|
From the repo root:
Sandcastle suggestions look to be working fine though. 
|
|
@ggetz Updated, please take another look. I think it should be good to go |
|
Thanks @jjspace, especially for your patience! 😬 |
Description
Major changes/focus for this PR:
modulethat should importCesiumandSandcastledirectlyimportstatements are not in the sandcastle code they will be added before loaded into thebucket. AFAICT this should account for all existing sandcastles that relied on the global objectswindowscope anymorewindow.startupfunction specifically to account for modules loading out of order. Switching totype=moduleeliminates thatCesiumandSandcastleregardless of the importsindex.htmlis the html/css code for a sandcastle,main.jsis the JS code for a sandcastle,sandcastle.yamlholds the metadata and is what dictates that a subfolder is even a gallery example in the first placeindex.htmldoes not contain the full page structure withheadandbodytags. This means you can no longer load gallery items directly with a path likelocalhost:8080/Apps/Sandcastle/gallery/3D Models.html.buildGallery.jshas been created to parse through the new gallery structure and generate a top levellist.jsonfile. This also does a little basic validation. Where this script lives and exactly what it does is still a little up in the airTODO:
create a "conversion script" to transform all our existing gallery examplesMostly done, will be separate PRIssue number and link
Part of #12566
Testing plan
node buildGallery.jsto generate the list filenpm run devand from the normal servernpm startafternpm run buildAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change