-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Sandcastle using Stratakit #12731
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
base: sandcastle-v2
Are you sure you want to change the base?
Sandcastle using Stratakit #12731
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
Any of this can (and probably should) be addressed in a future PR, but here's UI feedback from playing around:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jjspace! A few comments.
"allotment": { | ||
"use-resize-observer": { | ||
"react": "^19.0.0", | ||
"react-dom": "^19.0.0" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this all go in packages/sandcastle/package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very annoyingly npm seems to ignore overrides in workspaces in monorepos. It didn't work until I added it here. I can try and dig into it more but I think this is simply a limitation we have to work around until the dependency itself gets updated (ZeeCoder/use-resize-observer#108) or allotment
removes the dependency in favor of a "pure react" solution (johnwalley/allotment#833 (comment))
|
||
//const viewer = new Cesium.Viewer("cesiumContainer"); | ||
|
||
console.log("Sandcastle loaded"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to keep this around, or clean it up before merging the PR?
I added a "viewerless" sandcastle for now to help when testing to not always have to load the viewer and WebGL context
If the former, I would probably suggest leaving it entirely blank.
@@ -16,25 +16,31 @@ body { | |||
height: 100%; | |||
overflow: hidden; | |||
|
|||
background-color: white; | |||
background-color: var(--stratakit-color-bg-page-base); | |||
background-image: var(--_rings), var(--_gradient); | |||
|
|||
/* TODO: these should pull from the stratakit directly somehow eventually */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this TODO now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I specifically left the pure CSS integration of stratakit into the bucket iframe for a separate PR. This will be addressed with that work.
toggleExpanded: () => void; | ||
}) { | ||
const logsRef = useRef<HTMLDivElement>(document.createElement("div")); | ||
// TODO: determine if we need this lib or can implement ourselves. It's a little outdated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address or add an item to the running TODO list?
const knownLabels = useMemo(() => { | ||
const labels = new Set<string>(); | ||
for (const demo of demos) { | ||
demo.labels?.forEach((label) => labels.add(label)); | ||
} | ||
return [...labels].sort((a, b) => a.localeCompare(b)); | ||
}, [demos]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider aggregating these during the buildGallery
script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed in the search PR in favor of the list generated by pagefind
. I'm gonna leave as is for this PR.
flex-shrink: 1; | ||
/* Monaco needs a defined height for dynamic resizing to work */ | ||
--tabs-height: 1.5rem; | ||
height: calc(100% - var(--tabs-height)) !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the !important
somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc I added this because the monaco editor adds the height on the inline style tag which has "max priority" for styles. I'll take a peek at it again but I don't think so
|
||
background-image: var(--_rings), var(--_gradient); | ||
|
||
/* TODO: these should pull from the stratakit directly somehow eventually */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove?
define: { | ||
__COMMIT_SHA__: JSON.stringify(undefined), | ||
__CESIUM_VERSION__: JSON.stringify(`Cesium ${rootPackageJson.version}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment that may be easier to address later, but we may be able to remove some of the build complexity by grabbing this info during the buildGallery
step.
Description
The purpose of this PR was to do a full facelift to the new version of Sandcastle and embrace the Stratakit UI components. I also focused on solidifying the behavior of the various panels and parts of the app. It should now be very close to how we want it based on our latest designs.
What this PR does
App
component to include a header and application/platform of left navigationlocalStorage
based settings with the theme buttonWhat this PR does NOT do
Issue number and link
Part of #12566
Testing plan
npm run dev
from the Sandcastle package ornpm run build-sandcastle
andnpm start
from the rootconsole.log
,console.warn
andconsole.error
to make sure it displays them all correctlyid
starts on the editorAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change