Skip to content

New sandcastle title and dirty state tracking #12794

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

Open
wants to merge 3 commits into
base: sandcastle-v2
Choose a base branch
from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Aug 1, 2025

Description

  • Clean up the window title management. Now shows the name of the sandcastle or "New sandcastle" and "Cesium Sandcastle"
  • Add "dirty" state tracking for the code. When dirty add a * to the page title
  • Warn when navigating away from the page or trying to load a different gallery item if the code is "dirty"
    • "Saving" the code by clicking "Share" which currently updates the url in place is considered "clean" again because forward/back navigation will work to that URL. This behavior may change when the share functionality gets improved.
  • Slightly updated how the version string in the top right is constructed, it was broken before.

Merge after #12755 as this is based on the UI work in there

Issue number and link

Part of #12566

Testing plan

  • Run npm run dev in the sandcastle package or npm run build-sandcastle and npm start from the project root
  • load new sandcastle and check the title
  • Swap to a gallery demo and check the title
  • Make a change to the code and make sure the title gets it's *
  • Try navigating away when in the "dirty" state and make sure it always shows a warning
    • Reload the page
    • Click the top logo
    • Close the tab/window
    • Click a different gallery demo
    • Click the code button for a different demo

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz August 1, 2025 20:34
Copy link

github-actions bot commented Aug 1, 2025

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace jjspace force-pushed the window-title-dirty-state branch from c0ea093 to 6341032 Compare August 1, 2025 20:44
@jjspace jjspace self-assigned this Aug 8, 2025
Base automatically changed from gallery-search to sandcastle-v2 August 8, 2025 15:59
Copy link
Contributor

@ggetz ggetz left a 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 questions and branding-related comments.

@@ -4,7 +4,7 @@
<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/favicon.ico" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Sandcastle Reborn</title>
<title>Cesium Sandcastle</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<title>Cesium Sandcastle</title>
<title>CesiumJS Sandcastle</title>

I think, technically speaking, this should be the branding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly? I was just matching what the current Sandcastle uses

Comment on lines +285 to +287
return window.confirm(
"You have unsaved changes. Are you sure you want to navigate away from this demo?",
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally up-to-par with what we had before, but what do you think of propagating the current sandcastle to local storage or similar?

The goal would to be to persist state, like what happens when you reload a GitHub tab with an in-progress comment. Or maybe a slightly different workflow, like what happens if you leave and come back to https://geojson.io/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think of propagating the current sandcastle to local storage or similar?

This (or similar) has been requested often and I do really want to find some solution to try and prevent lost work. However I think it should be a separate effort, this PR just tries to match the functionality of the existing sandcastle. This is already on the running task list but further down for lower priority.happy to re-evaluate if you think it's a "must have" before release.

My current thought is not to make it truly persist every page load but offer a "saved state" that we can suggest the user reload like jsfiddle does

image

envString = `${host.replace("localhost:", "")} `;
}

const baseTitle = "Cesium Sandcastle";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const baseTitle = "Cesium Sandcastle";
const baseTitle = "CesiumJS Sandcastle";

or possibly even just the following due to all the other info?

Suggested change
const baseTitle = "Cesium Sandcastle";
const baseTitle = "Sandcastle";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want this to match whatever title we pick for the base page so the specific sandcastle title is just prepended. I'm open to dropping down to just "Sandcastle" but I wasn't sure about dropping the branding

Comment on lines +272 to +276
if (title !== "") {
document.title = `${envString}${title}${dirtyIndicator} | ${baseTitle}`;
} else {
document.title = `${envString}${baseTitle}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Why shouldn't we show the dirty indicator on named sandcastles?
  2. Assuming we can, couldn't we simplify this to thew following?
Suggested change
if (title !== "") {
document.title = `${envString}${title}${dirtyIndicator} | ${baseTitle}`;
} else {
document.title = `${envString}${baseTitle}`;
}
document.title = `${envString}${title}${dirtyIndicator} | ${baseTitle}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check makes sure that we only add the title and the | to the page title when there's actually a title to prepend in front of the bar. The page starts with an empty title "" in state when it initially loads because we don't know what it will be yet.
If a specific gallery demo is not loaded then we replace it with "New Sandcastle" thus providing a title and it will show the dirty indicator.

@jjspace
Copy link
Contributor Author

jjspace commented Aug 8, 2025

@ggetz no updates yet but I responded to all your comments if you wanna take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants